In D29042#650907, @ehem_freebsd_m5p.com wrote:

Yeah, I've got no idea what-so-ever why one might feel xenpv.c should be
in a "bus" directory. No similarity to any type of bus. No devices
attached at all. Certainly not a bus.

Also, the 'Adapted-to-HEAD-by' tag is not really a standard tag, and doesn't mention the original author either. I would just add a line saying:

LGTM, we should make this a bool type also.

Some minor comments.

LGTM.

I would consider placing it in sys/dev/xen/bus/xenpv.c

Do you have a git repo I can pick this patch from? So that it has the proper Author field. Also in FreeBSD we tend to use the 'Sponsored by:' tag to credit the company that paid for the work (if any):

Don't really have much knowledge in this area to provide a meaningful review I'm afraid.

Works for me, now the font seems fine on a 2160p monitor, and there's no black bar at the bottom of the screen.

I have some comments, could you please send this upstream to xen-devel against xen.git? I don't plan to carry custom rc.d scripts in the package. If you don't feel comfortable sending it yourself I can pick it up.

In D28693#644531, @debdrup wrote:

The parentheses just seem to be making use of two of the 72 columns which we have until we need to add a newline.
Do we need them?

• Move bootmethod declaration to x86_var.h.
• Remove extern declaration from pv.c.

• Move bootmethod definition to x86/cpu_machdep.c.
• Declare it in md_var.h.

@tsoome let me know if you are happy with this version, as I would like to commit it soon-ish. Thanks!

Other projects I work with use the Fixes tag with the following format:

• Fix module unload.
• Make module depend on efirt.

• Add comment in efi_ops struct to describe the intended usage.
• Introduce active_efi_ops as a pointer to the currently in-use efi_ops.

Thanks for the comment!

For the reference, the user of this new interface is at: https://reviews.freebsd.org/D28621

Please send the patch to xen-devel@lists.xenproject.org against the upstream Xen repository (http://xenbits.xen.org/gitweb/?p=xen.git;a=summary) (ie: place it in tools/hotplug/FreeBSD/rc.d/xendomains.in). It makes no sense to carry a custom script in the port when we can have it added upstream.

• Introduce a mb2hdr struct to pass information between the load and the exec functions.
• Bail out at load if there's no keep BS tag.
• Add the tags we unconditionally set to the check in MULTIBOOT_HEADER_TAG_INFORMATION_REQUEST.

Thanks for the review! Will try to fix the comments later and update.

Fix comments by tsoome.

Expand the list of file patterns to ignore.

In D28095#627811, @lwhsu wrote:

?cscope.out is removed, is that intentional?

I wonder whether to also add '~.' to the list of patterns.

LGTM. I've cheked the rules and the new format seems to match them. Note I'm not a linguist at all. Do you know which complaints the original submitter had?

In D6611#580317, @gallatin wrote:

In D6611#580267, @jhb wrote:

I think one question is how can you really identify these packets? You can send a packet into a VM (e.g. via an if_tap interface from a localhost connection) but that VM might itself forward the packet internally to another interface that then heads out onto the wire. You can't really know that before you send the packet into the if_tap, and the VM itself doesn't know which interfaces are internal and which are not.

I considered & rejected some magic solution where we constrain virtual interface mac addresses to a range that is unique and which can be easily checked. Then we'd know if a packet came in on a virtual interface & needs checksumming. But that doesn't work if the VM has pass-thru access to a physical NIC and can send a packet w/o going through the host.

In D6656#578863, @gallatin wrote:

In D6656#578326, @gallatin wrote:

I'm confused. t_maxsegsize is basically the connection's MSS, eg, what the NIC is supposed to break the TSO into. However if_hw_tsomaxsegsize is the max size of the TSO to be broken up. Eg, on a NIC without oddball limitations, if_hw_tsomaxsegcount might be 64, if_hw_tsomaxsegsize might be 65535. That means that TSO is supposed to send a max of 64K down to the NIC, in a max of 64 packets. If t_maxseg is less than 1000, then we'd have to limit the TSO to 64 packets.

It seems like by restricting if_hw_tsomaxsegsize to t_maxseg, you're essentially turning off TSO, since you're only allowing one MSS to be sent at a time.

Ugh, I'm wrong. I was confusing t_tsomax with t_tsomaxsegsize. I forgot that some NICs are apparently so broken that they limit TSO seg sizes.

In D6611#578858, @gallatin wrote:

This will break some NICs (see comments on D6656 -- basically, if you're setting CSUM_TCP then you need to replace the TCP checksum with the pseudohdr checksum, and store the offset of the TCP header's checksum in csum_data).

The important question is: If the packet has already had its checksum checked, then the checksum is correct. Why not update the checksum normally and send the packet without checksum offload enabled?

In D6611#578198, @rgrimes wrote:

Is there any concern or additional work needed for ipV6?