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

[Alexey G]

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

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

[Roger Pau Monné]

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 
> ---
>  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_824410x1237
> +#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 
>  #include 
>  #include 
> @@ -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_Q352
> +#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

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

[Wei Liu]

On Wed, Mar 14, 2018 at 03:58:17AM +1000, Alexey G wrote:
> On Tue, 13 Mar 2018 17:26:04 +
> Wei Liu  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 
> >> ---
> >>  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_824410x1237
> >> +#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

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

[Alexey G]

On Tue, 13 Mar 2018 17:26:04 +
Wei Liu  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 
>> ---
>>  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_824410x1237
>> +#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.

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

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

[Wei Liu]

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 
> ---
>  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_824410x1237
> +#define PCI_DEVICE_ID_INTEL_Q35_MCH  0x29c0
> +
> +

Too many blank lines.

>  #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 
>  #include 
>  #include 
> @@ -735,6 +736,52 @@ void __bug(char *file, int line)
>  crash();
>  }
>  
> +
> +static int machine_type = MACHINE_TYPE_UNDEFINED;
> +
> +int get_pc_machine_type(void)
> +{
> +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)

Coding style.

> +{
> +switch (device_id)

Ditto.

And this patch should be folded into its user, unless the patch that
uses it is very big on its own.

Wei.

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

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

[Alexey Gerasimenko]

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 
---
 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_824410x1237
+#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 
 #include 
 #include 
@@ -735,6 +736,52 @@ void __bug(char *file, int line)
 crash();
 }
 
+
+static int machine_type = MACHINE_TYPE_UNDEFINED;
+
+int get_pc_machine_type(void)
+{
+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)
+{
+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();
+}
+
+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_Q352
+#define MACHINE_TYPE_UNKNOWN(-1)
+
+int get_pc_machine_type(void);
+
 /* Get a pointer to the shared-info page */
 struct shared_info *get_shared_info(void) __attribute__ ((const));
 
-- 
2.11.0


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