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

["Roger Pau Monné" <roger.pau@citrix.com>]

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.

This is not correct, the return values are strictly MACHINE_TYPE_I440
or MACHINE_TYPE_Q35. Everything else ends up in a BUG().

Also makes me wonder whether this should instead be init_machine_type,
and users should just read machine_type directly.

> 
> 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
> +
> +
>  #endif /* __HVMLOADER_PCI_REGS_H__ */
>  
>  /*
> diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
> index 0c3f2d24cd..5739a87628 100644
> --- a/tools/firmware/hvmloader/util.c
> +++ b/tools/firmware/hvmloader/util.c
> @@ -22,6 +22,7 @@
>  #include "hypercall.h"
>  #include "ctype.h"
>  #include "vnuma.h"
> +#include "pci_regs.h"
>  #include <acpi2_0.h>
>  #include <libacpi.h>
>  #include <stdint.h>
> @@ -735,6 +736,52 @@ void __bug(char *file, int line)
>      crash();
>  }
>  
> +
> +static int machine_type = MACHINE_TYPE_UNDEFINED;

There's no need to init this, _UNDEFINED is 0 which is the default
value.

> +
> +int get_pc_machine_type(void)

You introduce a function that's not used anywhere, and the commit log
doesn't mention why this is needed at all. In general I prefer
functions to be introduced with at least a caller, or else it needs to
be described in the commit message why this is not the case.

> +{
> +    uint16_t vendor_id;
> +    uint16_t device_id;
> +
> +    if (machine_type != MACHINE_TYPE_UNDEFINED)
> +        return machine_type;
> +
> +    machine_type = MACHINE_TYPE_UNKNOWN;
> +
> +    vendor_id = pci_readw(0, PCI_VENDOR_ID);
> +    device_id = pci_readw(0, PCI_DEVICE_ID);
> +
> +    /* only Intel platforms are emulated currently */
> +    if (vendor_id == PCI_VENDOR_ID_INTEL)

Should this maybe be a BUG_ON(vendor_id != PCI_VENDOR_ID_INTEL) then?
Note that in this case you end up with a BUG later anyway.

> +    {
> +        switch (device_id)
> +        {
> +        case PCI_DEVICE_ID_INTEL_82441:
> +            machine_type = MACHINE_TYPE_I440;
> +            printf("Detected i440 chipset\n");
> +            break;
> +
> +        case PCI_DEVICE_ID_INTEL_Q35_MCH:
> +            machine_type = MACHINE_TYPE_Q35;
> +            printf("Detected Q35 chipset\n");
> +            break;
> +
> +        default:
> +            break;
> +        }
> +    }
> +
> +    if (machine_type == MACHINE_TYPE_UNKNOWN)
> +    {
> +        printf("Unknown emulated chipset encountered, VID=%04Xh, DID=%04Xh\n",
> +               vendor_id, device_id);
> +        BUG();

Why not place this in the default switch label? That would allow you
to get rid of the MACHINE_TYPE_UNKNOWN define also.

> +    }
> +
> +    return machine_type;
> +}
> +
>  static void validate_hvm_info(struct hvm_info_table *t)
>  {
>      uint8_t *ptr = (uint8_t *)t;
> diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
> index 7bca6418d2..7c77bedb00 100644
> --- a/tools/firmware/hvmloader/util.h
> +++ b/tools/firmware/hvmloader/util.h
> @@ -100,6 +100,14 @@ void pci_write(uint32_t devfn, uint32_t reg, uint32_t len, uint32_t val);
>  #define pci_writew(devfn, reg, val) pci_write(devfn, reg, 2, (uint16_t)(val))
>  #define pci_writel(devfn, reg, val) pci_write(devfn, reg, 4, (uint32_t)(val))
>  
> +/* Emulated machine types */
> +#define MACHINE_TYPE_UNDEFINED      0
> +#define MACHINE_TYPE_I440           1
> +#define MACHINE_TYPE_Q35            2
> +#define MACHINE_TYPE_UNKNOWN        (-1)

An enum seems better suited for this.

Thanks, Roger.

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