Re: [RFC PATCH 03/12] hvmloader: add function to query an emulated machine type (i440/Q35)

[Wei Liu <wei.liu2@citrix.com>]

On Wed, Mar 14, 2018 at 03:58:17AM +1000, Alexey G wrote:
> On Tue, 13 Mar 2018 17:26:04 +0000
> Wei Liu <wei.liu2@citrix.com> wrote:
> 
> >On Tue, Mar 13, 2018 at 04:33:48AM +1000, Alexey Gerasimenko wrote:
> >> This adds a new function get_pc_machine_type() which allows to
> >> determine the emulated chipset type. Supported return values:
> >> 
> >> - MACHINE_TYPE_I440
> >> - MACHINE_TYPE_Q35
> >> - MACHINE_TYPE_UNKNOWN, results in the error message being printed
> >>   followed by calling BUG() in hvmloader.
> >> 
> >> Signed-off-by: Alexey Gerasimenko <x1917x@gmail.com>
> >> ---
> >>  tools/firmware/hvmloader/pci_regs.h |  5 ++++
> >>  tools/firmware/hvmloader/util.c     | 47
> >> +++++++++++++++++++++++++++++++++++++
> >> tools/firmware/hvmloader/util.h     |  8 +++++++ 3 files changed, 60
> >> insertions(+)
> >> 
> >> diff --git a/tools/firmware/hvmloader/pci_regs.h
> >> b/tools/firmware/hvmloader/pci_regs.h index 7bf2d873ab..ba498b840e
> >> 100644 --- a/tools/firmware/hvmloader/pci_regs.h
> >> +++ b/tools/firmware/hvmloader/pci_regs.h
> >> @@ -107,6 +107,11 @@
> >>  
> >>  #define PCI_INTEL_OPREGION 0xfc /* 4 bits */
> >>  
> >> +#define PCI_VENDOR_ID_INTEL              0x8086
> >> +#define PCI_DEVICE_ID_INTEL_82441        0x1237
> >> +#define PCI_DEVICE_ID_INTEL_Q35_MCH      0x29c0
> >> +
> >> +  
> >
> >Too many blank lines.
> 
> Will fix.
> 
> >> @@ -735,6 +736,52 @@ void __bug(char *file, int line)
> >>      crash();
> >>  }
> >>  
> >> +    /* only Intel platforms are emulated currently */
> >> +    if (vendor_id == PCI_VENDOR_ID_INTEL)  
> >
> >Coding style.
> >
> >Ditto.
> 
> Will fix.
> 
> >And this patch should be folded into its user, unless the patch that
> >uses it is very big on its own.
> 
> Hmm, looks like I overfollowed the recommendation about making atomic
> patches for easier review. There are multiple users of these function,
> it was made in a separate patch just because of this. In the next
> version I'll merge it with some of the patches which use this function
> then.

It really depends. It will take some back-and-forth to find the right
balance. I can't say I'm very consistent on this either.

If you think leaving it in a separate patch is better, I won't object.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel