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

[Alexey G <x1917x@gmail.com>]

On Mon, 19 Mar 2018 12:56:51 +0000
Roger Pau Monné <roger.pau@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.  
>
>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.

Completely agree here, get_-style function should normally return a
value, not to perform extra checks and call BUG().

Renaming the function to init_machine_type() and replacing
get_pc_machine_type() usage to reading the machine_type (extern)
variable should be more clear (or, perhaps, one-line function to return
its value).

This way we can assume the machine type was successfully validated,
hence no need for additional checks for MACHINE_TYPE_UNKNOWN value (and
the MACHINE_TYPE_UNKNOWN itself).

>>  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

>> +static int machine_type = MACHINE_TYPE_UNDEFINED;  
>
>There's no need to init this, _UNDEFINED is 0 which is the default
>value.

Using the explicit initialization with the named constant here merely
improves readability. Comparing the enum-style variable later with
MACHINE_TYPE_UNDEFINED seems better than comparing it with 0. It's zero
difference for a compiler, but makes difference for a human. :)

Besides, it will be converted to enum type anyway, so some named entry
for the 'unassigned' value will be appropriate I think. 

>> +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.

There are multiple users, will merge the function with some
of its callers (Wei suggested the same).

>> +{
>> +    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.

Yes, this is intentional. Non-Intel vendor => unknown machine.

>> +    {
>> +        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.

This check outside the switch covers cases (Vendor is not Intel)
OR (Vendor is Intel but the host bridge is unknown).

I guess it can be moved into the switch, but it means there will
be two copies of printf(VID:DID)/BUG() block -- one for Vendor ID check,
second is for Device ID processing. Placing this check outside allows
to reuse it for both cases.

>> +    }
>> +
>> +    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.

Agree, + MACHINE_TYPE_UNKNOWN will be dropped in the next version.

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