Since the addition of the MSR_AMD64_DR{1-4}_ADDRESS_MASK MSRs to the
msrs_to_send array, the calculations for the maximum number of MSRs that
the hypercall can handle is off by 4.

Remove the addition of 4 to the maximum number of MSRs that
XEN_DOMCTL_{set,get}_vcpu_msrs supports, as those are already part of the
array.

A further adjustment could be to subtract 4 from the maximum size if the DBEXT
CPUID feature is not exposed to the guest, but guest_{rd,wr}msr() will already
perform that check when fetching or loading the MSRs.  The maximum array is
used to indicate the caller of the buffer it needs to allocate in the get case,
and as an early input sanitation in the set case, using a buffer size slightly
lager than required is not an issue.

Fixes: 86d47adcd3c4 ('x86/msr: Handle MSR_AMD64_DR{0-3}_ADDRESS_MASK in the new MSR infrastructure')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
I'm tempted to just get rid of nr_msrs and use ARRAY_SIZE(msrs_to_send)
instead, but refrained from doing it.
---
 xen/arch/x86/domctl.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index f76de5be9437..37ebcb3abbc7 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1088,10 +1088,6 @@ long arch_do_domctl(
              !is_pv_domain(d) )
             break;
 
-        /* Count maximum number of optional msrs. */
-        if ( boot_cpu_has(X86_FEATURE_DBEXT) )
-            nr_msrs += 4;
-
         if ( domctl->cmd == XEN_DOMCTL_get_vcpu_msrs )
         {
             ret = 0; copyback = true;
-- 
2.46.0

On 08.10.2024 10:37, Roger Pau Monne wrote:
> Since the addition of the MSR_AMD64_DR{1-4}_ADDRESS_MASK MSRs to the
> msrs_to_send array, the calculations for the maximum number of MSRs that
> the hypercall can handle is off by 4.
> 
> Remove the addition of 4 to the maximum number of MSRs that
> XEN_DOMCTL_{set,get}_vcpu_msrs supports, as those are already part of the
> array.
> 
> A further adjustment could be to subtract 4 from the maximum size if the DBEXT
> CPUID feature is not exposed to the guest, but guest_{rd,wr}msr() will already
> perform that check when fetching or loading the MSRs.  The maximum array is
> used to indicate the caller of the buffer it needs to allocate in the get case,
> and as an early input sanitation in the set case, using a buffer size slightly
> lager than required is not an issue.
> 
> Fixes: 86d47adcd3c4 ('x86/msr: Handle MSR_AMD64_DR{0-3}_ADDRESS_MASK in the new MSR infrastructure')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

> ---
> I'm tempted to just get rid of nr_msrs and use ARRAY_SIZE(msrs_to_send)
> instead, but refrained from doing it.

I think the variable would indeed better go away now. Yet that doesn't
necessarily need to happen right here.

Jan

Some error paths in the MSR state loading logic don't contain error messages,
which makes debugging them quite hard without adding extra patches to print the
information.

Add two new log messages to the MSR state load path that print information
about the entry that failed to load, for both PV and HVM.

While there also adjust XEN_DOMCTL_set_vcpu_msrs to return -ENXIO in case the
MSR is unhandled or can't be loaded, so it matches the error code used by HVM
MSR loading (and it's less ambiguous than -EINVAL).

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Add messages to PV MSR loading.
 - Return -ENXIO from PV loading path also.
 - Do not print the return code from guest_wrmsr(), it's not specially helpful
   given the current logic, as it will always be X86EMUL_EXCEPTION in case of
   failure.
---
 xen/arch/x86/domctl.c  | 8 ++++++++
 xen/arch/x86/hvm/hvm.c | 7 +++++++
 2 files changed, 15 insertions(+)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 96d816cf1a7d..f76de5be9437 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1166,6 +1166,7 @@ long arch_do_domctl(
                 if ( msr.reserved )
                     break;
 
+                ret = -ENXIO;
                 switch ( msr.index )
                 {
                 case MSR_SPEC_CTRL:
@@ -1174,9 +1175,16 @@ long arch_do_domctl(
                 case MSR_AMD64_DR0_ADDRESS_MASK:
                 case MSR_AMD64_DR1_ADDRESS_MASK ... MSR_AMD64_DR3_ADDRESS_MASK:
                     if ( guest_wrmsr(v, msr.index, msr.value) != X86EMUL_OKAY )
+                    {
+                        printk(XENLOG_G_ERR
+                               "%pv load MSR %#x with value %#lx failed\n",
+                               v, msr.index, msr.value);
                         break;
+                    }
                     continue;
                 }
+                printk(XENLOG_G_ERR "%pv attempted load of unhandled MSR %#x\n",
+                       v, msr.index);
                 break;
             }
 
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 69a25571db8d..200f0a414138 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1598,10 +1598,17 @@ static int cf_check hvm_load_cpu_msrs(struct domain *d, hvm_domain_context_t *h)
             rc = guest_wrmsr(v, ctxt->msr[i].index, ctxt->msr[i].val);
 
             if ( rc != X86EMUL_OKAY )
+            {
+                printk(XENLOG_G_ERR
+                       "HVM %pv load MSR %#x with value %#lx failed\n",
+                       v, ctxt->msr[i].index, ctxt->msr[i].val);
                 return -ENXIO;
+            }
             break;
 
         default:
+            printk(XENLOG_G_ERR "HVM %pv attempted load of unhandled MSR %#x\n",
+                   v, ctxt->msr[i].index);
             return -ENXIO;
         }
     }
-- 
2.46.0

I've wrongly send this together with "x86/domctl: fix maximum number
of MSRs in XEN_DOMCTL_{get,set}_vcpu_msrs".  There's a separate email
with just "[PATCH v2] x86/msr: add log messages to MSR state load
error paths".  Both have the same contents.

Roger.