[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH 03/12] hvmloader: add function to query an emulated machine type (i440/Q35)
On Mon, 19 Mar 2018 12:56:51 +0000 Roger Pau Monné <roger.pau@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |