| Message ID | 1498612044-14114-3-git-send-email-marmarek@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> 06/28/17 3:09 AM >>> >It's bit 9 not 10 (which is ibs). Indeed, but shouldn't it rather be removed? We don't expose it from the hypervisor at all anymore: XEN_CPUFEATURE(OSVW, 3*32+ 9) /* OS Visible Workaround */ (note the absence of any marker character immediately inside the comment). Jan
On 28/06/17 07:09, Jan Beulich wrote: >>>> Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> 06/28/17 3:09 AM >>> >> It's bit 9 not 10 (which is ibs). > Indeed, but shouldn't it rather be removed? We don't expose it from the > hypervisor at all anymore: > > XEN_CPUFEATURE(OSVW, 3*32+ 9) /* OS Visible Workaround */ > > (note the absence of any marker character immediately inside the comment). I don't believe we have ever actually offered OSVW to guests, despite the pretence of being able to. ISTR it was always clobbered before being given to a guest. Having said that, we should be advertising OSVW. It's entire purpose is to tell the OS that there is something it can do to manually work round a specific erratum. OTOH, making this migrate safe is liable to be very complicated... ~Andrew
On Wed, Jun 28, 2017 at 11:16:33AM +0100, Andrew Cooper wrote: > On 28/06/17 07:09, Jan Beulich wrote: > >>>> Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> 06/28/17 3:09 AM >>> > >> It's bit 9 not 10 (which is ibs). > > Indeed, but shouldn't it rather be removed? We don't expose it from the > > hypervisor at all anymore: > > > > XEN_CPUFEATURE(OSVW, 3*32+ 9) /* OS Visible Workaround */ > > > > (note the absence of any marker character immediately inside the comment). > > I don't believe we have ever actually offered OSVW to guests, despite > the pretence of being able to. ISTR it was always clobbered before > being given to a guest. > > Having said that, we should be advertising OSVW. It's entire purpose is > to tell the OS that there is something it can do to manually work round > a specific erratum. OTOH, making this migrate safe is liable to be very > complicated... I don't have opinion on either approach here, but the current state is clearly wrong. You've got two versions of the patch, choose one ;)
On 28/06/17 17:14, Marek Marczykowski-Górecki wrote: > On Wed, Jun 28, 2017 at 11:16:33AM +0100, Andrew Cooper wrote: >> On 28/06/17 07:09, Jan Beulich wrote: >>>>>> Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> 06/28/17 3:09 AM >>> >>>> It's bit 9 not 10 (which is ibs). >>> Indeed, but shouldn't it rather be removed? We don't expose it from the >>> hypervisor at all anymore: >>> >>> XEN_CPUFEATURE(OSVW, 3*32+ 9) /* OS Visible Workaround */ >>> >>> (note the absence of any marker character immediately inside the comment). >> I don't believe we have ever actually offered OSVW to guests, despite >> the pretence of being able to. ISTR it was always clobbered before >> being given to a guest. >> >> Having said that, we should be advertising OSVW. It's entire purpose is >> to tell the OS that there is something it can do to manually work round >> a specific erratum. OTOH, making this migrate safe is liable to be very >> complicated... > I don't have opinion on either approach here, but the current state is > clearly wrong. You've got two versions of the patch, choose one ;) > I'd prefer this version of the patch, so it doesn't suddenly remove a piece of libxl API, but it is up to Wei / Ian at the end of the day. One option which was being discussed in the context of my CPUID (part 3) work was to automatically this list of keywords. ~Andrew
On Wed, Jun 28, 2017 at 05:20:38PM +0100, Andrew Cooper wrote: > On 28/06/17 17:14, Marek Marczykowski-Górecki wrote: > > On Wed, Jun 28, 2017 at 11:16:33AM +0100, Andrew Cooper wrote: > >> On 28/06/17 07:09, Jan Beulich wrote: > >>>>>> Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> 06/28/17 3:09 AM >>> > >>>> It's bit 9 not 10 (which is ibs). > >>> Indeed, but shouldn't it rather be removed? We don't expose it from the > >>> hypervisor at all anymore: > >>> > >>> XEN_CPUFEATURE(OSVW, 3*32+ 9) /* OS Visible Workaround */ > >>> > >>> (note the absence of any marker character immediately inside the comment). > >> I don't believe we have ever actually offered OSVW to guests, despite > >> the pretence of being able to. ISTR it was always clobbered before > >> being given to a guest. > >> > >> Having said that, we should be advertising OSVW. It's entire purpose is > >> to tell the OS that there is something it can do to manually work round > >> a specific erratum. OTOH, making this migrate safe is liable to be very > >> complicated... > > I don't have opinion on either approach here, but the current state is > > clearly wrong. You've got two versions of the patch, choose one ;) > > > > I'd prefer this version of the patch, so it doesn't suddenly remove a > piece of libxl API, but it is up to Wei / Ian at the end of the day. Keeping it is better. It doesn't really make any difference to the guest but some tools might complain if we remove it. > > One option which was being discussed in the context of my CPUID (part 3) > work was to automatically this list of keywords. > "generate"? And yes, I fully support this work. ;-) > ~Andrew
On 28/06/17 18:51, Wei Liu wrote: > On Wed, Jun 28, 2017 at 05:20:38PM +0100, Andrew Cooper wrote: >> On 28/06/17 17:14, Marek Marczykowski-Górecki wrote: >>> On Wed, Jun 28, 2017 at 11:16:33AM +0100, Andrew Cooper wrote: >>>> On 28/06/17 07:09, Jan Beulich wrote: >>>>>>>> Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> 06/28/17 3:09 AM >>> >>>>>> It's bit 9 not 10 (which is ibs). >>>>> Indeed, but shouldn't it rather be removed? We don't expose it from the >>>>> hypervisor at all anymore: >>>>> >>>>> XEN_CPUFEATURE(OSVW, 3*32+ 9) /* OS Visible Workaround */ >>>>> >>>>> (note the absence of any marker character immediately inside the comment). >>>> I don't believe we have ever actually offered OSVW to guests, despite >>>> the pretence of being able to. ISTR it was always clobbered before >>>> being given to a guest. >>>> >>>> Having said that, we should be advertising OSVW. It's entire purpose is >>>> to tell the OS that there is something it can do to manually work round >>>> a specific erratum. OTOH, making this migrate safe is liable to be very >>>> complicated... >>> I don't have opinion on either approach here, but the current state is >>> clearly wrong. You've got two versions of the patch, choose one ;) >>> >> I'd prefer this version of the patch, so it doesn't suddenly remove a >> piece of libxl API, but it is up to Wei / Ian at the end of the day. > Keeping it is better. It doesn't really make any difference to the guest > but some tools might complain if we remove it. > >> One option which was being discussed in the context of my CPUID (part 3) >> work was to automatically this list of keywords. >> > "generate"? Oops. Yes. > And yes, I fully support this work. ;-) It's not a fully-formed idea yet. It wouldn't easily deal with duplicate names, and won't deal with CPUID content other than the feature flags known to Xen at build time. I eventually want to be able to express things like cpuid="pmu=3" to enable vPMU emulating version 3. ~Andrew
diff --git a/tools/libxl/libxl_cpuid.c b/tools/libxl/libxl_cpuid.c index 1cf7973..a4a69af 100644 --- a/tools/libxl/libxl_cpuid.c +++ b/tools/libxl/libxl_cpuid.c @@ -203,7 +203,7 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str) {"skinit", 0x80000001, NA, CPUID_REG_ECX, 12, 1}, {"xop", 0x80000001, NA, CPUID_REG_ECX, 11, 1}, {"ibs", 0x80000001, NA, CPUID_REG_ECX, 10, 1}, - {"osvw", 0x80000001, NA, CPUID_REG_ECX, 10, 1}, + {"osvw", 0x80000001, NA, CPUID_REG_ECX, 9, 1}, {"3dnowprefetch",0x80000001, NA, CPUID_REG_ECX, 8, 1}, {"misalignsse", 0x80000001, NA, CPUID_REG_ECX, 7, 1}, {"sse4a", 0x80000001, NA, CPUID_REG_ECX, 6, 1},
It's bit 9 not 10 (which is ibs). Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- tools/libxl/libxl_cpuid.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)