Re: [PATCH 4/4] tools/xen-mceinj: Pass in GPA when injecting through MSR_MCI_ADDR

["Egger, Christoph" <chegger@amazon.de>]

On 2015/09/15 12:28, Haozhong Zhang wrote:
> On Tue, Sep 15, 2015 at 12:17:01PM +0200, Egger, Christoph wrote:
>> On 2015/09/15 10:29, Haozhong Zhang wrote:
>>> This patch removes the address translation in xen-mceinj which
>>> translates the guest physical address passed-in through the argument
>>> of '-p' to the host machine address.
>>>
>>> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
>>
>> Comments inline.
>>
>>> ---
>>>  tools/tests/mce-test/tools/xen-mceinj.c | 135 +++++---------------------------
>>>  1 file changed, 19 insertions(+), 116 deletions(-)
>>>
>>> diff --git a/tools/tests/mce-test/tools/xen-mceinj.c b/tools/tests/mce-test/tools/xen-mceinj.c
>>> index 0c2b640..7b9dd13 100644
>>> --- a/tools/tests/mce-test/tools/xen-mceinj.c
>>> +++ b/tools/tests/mce-test/tools/xen-mceinj.c
>>> @@ -238,7 +238,8 @@ static int add_msr_intpose(xc_interface *xc_handle,
>>>                             uint32_t cpu_nr,
>>>                             uint32_t flags,
>>>                             uint64_t msr,
>>> -                           uint64_t val)
>>> +                           uint64_t val,
>>> +                           domid_t domid)
>>>  {
>>>      uint32_t count;
>>>
>>> @@ -256,6 +257,8 @@ static int add_msr_intpose(xc_interface *xc_handle,
>>>          msr_inj.mcinj_cpunr = cpu_nr;
>>>          msr_inj.mcinj_flags = flags;
>>>      }
>>> +    if ( flags & MC_MSRINJ_F_GPADDR )
>>> +        msr_inj.mcinj_domid = domid;
>>>      msr_inj.mcinj_msr[count].reg = msr;
>>>      msr_inj.mcinj_msr[count].value = val;
>>>      msr_inj.mcinj_count++;
>>> @@ -268,105 +271,15 @@ static int add_msr_bank_intpose(xc_interface *xc_handle,
>>>                                  uint32_t flags,
>>>                                  uint32_t type,
>>>                                  uint32_t bank,
>>> -                                uint64_t val)
>>> +                                uint64_t val,
>>> +                                domid_t domid)
>>>  {
>>>      uint64_t msr;
>>>
>>>      msr = bank_addr(bank, type);
>>>      if ( msr == INVALID_MSR )
>>>          return -1;
>>> -    return add_msr_intpose(xc_handle, cpu_nr, flags, msr, val);
>>> -}
>>> -
>>> -#define MCE_INVALID_MFN ~0UL
>>> -#define mfn_valid(_mfn) (_mfn != MCE_INVALID_MFN)
>>> -#define mfn_to_pfn(_mfn) (live_m2p[(_mfn)])
>>> -static uint64_t guest_mfn(xc_interface *xc_handle,
>>> -                               uint32_t domain,
>>> -                               uint64_t gpfn)
>>> -{
>>> -    xen_pfn_t *live_m2p = NULL;
>>> -    int ret;
>>> -    unsigned long hvirt_start;
>>> -    unsigned int pt_levels;
>>> -    uint64_t * pfn_buf = NULL;
>>> -    unsigned long max_mfn = 0; /* max mfn of the whole machine */
>>> -    unsigned long m2p_mfn0;
>>> -    unsigned int guest_width;
>>> -    long max_gpfn,i;
>>> -    uint64_t mfn = MCE_INVALID_MFN;
>>> -
>>> -    if ( domain > DOMID_FIRST_RESERVED )
>>> -        return MCE_INVALID_MFN;
>>> -
>>> -    /* Get max gpfn */
>>> -    max_gpfn = do_memory_op(xc_handle, XENMEM_maximum_gpfn, &domain,
>>> -                               sizeof(domain)) + 1;
>>> -    if ( max_gpfn <= 0 )
>>> -        err(xc_handle, "Failed to get max_gpfn 0x%lx", max_gpfn);
>>> -
>>> -    Lprintf("Maxium gpfn for dom %d is 0x%lx", domain, max_gpfn);
>>> -
>>> -    /* Get max mfn */
>>> -    if ( !get_platform_info(xc_handle, domain,
>>> -                            &max_mfn, &hvirt_start,
>>> -                            &pt_levels, &guest_width) )
>>> -        err(xc_handle, "Failed to get platform information");
>>> -
>>> -    /* Get guest's pfn list */
>>> -    pfn_buf = calloc(max_gpfn, sizeof(uint64_t));
>>> -    if ( !pfn_buf )
>>> -        err(xc_handle, "Failed to alloc pfn buf");
>>> -
>>> -    ret = xc_get_pfn_list(xc_handle, domain, pfn_buf, max_gpfn);
>>> -    if ( ret < 0 ) {
>>> -        free(pfn_buf);
>>> -        err(xc_handle, "Failed to get pfn list %x", ret);
>>> -    }
>>> -
>>> -    /* Now get the m2p table */
>>> -    live_m2p = xc_map_m2p(xc_handle, max_mfn, PROT_READ, &m2p_mfn0);
>>> -    if ( !live_m2p )
>>> -        err(xc_handle, "Failed to map live M2P table");
>>> -
>>> -    /* match the mapping */
>>> -    for ( i = 0; i < max_gpfn; i++ )
>>> -    {
>>> -        uint64_t tmp;
>>> -        tmp = pfn_buf[i];
>>> -
>>> -        if (mfn_valid(tmp) &&  (mfn_to_pfn(tmp) == gpfn))
>>> -        {
>>> -            mfn = tmp;
>>> -            Lprintf("We get the mfn 0x%lx for this injection", mfn);
>>> -            break;
>>> -        }
>>> -    }
>>> -
>>> -    munmap(live_m2p, M2P_SIZE(max_mfn));
>>> -
>>> -    free(pfn_buf);
>>> -    return mfn;
>>> -}
>>> -
>>> -static uint64_t mca_gpfn_to_mfn(xc_interface *xc_handle,
>>> -                                uint32_t domain,
>>> -                                uint64_t gfn)
>>> -{
>>> -    uint64_t index;
>>> -    long max_gpfn;
>>> -
>>> -    /* If domain is xen, means we want pass index directly */
>>> -    if ( domain == DOMID_XEN )
>>> -        return gfn;
>>> -
>>> -    max_gpfn = do_memory_op(xc_handle, XENMEM_maximum_gpfn, &domain,
>>> -                               sizeof(domain)) + 1;
>>> -    if ( max_gpfn <= 0 )
>>> -        err(xc_handle, "Failed to get max_gpfn 0x%lx", max_gpfn);
>>> -    index = gfn % max_gpfn;
>>> -
>>> -    return guest_mfn(xc_handle, domain, index);
>>> +    return add_msr_intpose(xc_handle, cpu_nr, flags, msr, val, domid);
>>>  }
>>>
>>>  static int inject_mcg_status(xc_interface *xc_handle,
>>> @@ -374,7 +287,7 @@ static int inject_mcg_status(xc_interface *xc_handle,
>>>                               uint64_t val)
>>>  {
>>>      return add_msr_intpose(xc_handle, cpu_nr, MC_MSRINJ_F_INTERPOSE,
>>> -                               MSR_IA32_MCG_STATUS, val);
>>> +                               MSR_IA32_MCG_STATUS, val, 0);
>>
>> Why do you hardcode 0 here?
>> I think you should pass domid in here so that the DomU sees the right
>> status value.
> 
> If the flag MC_MSRINJ_F_GPADDR is not given (which is the case here),
> the domain id is not used when do_mca() handles the MCE injection, so
> I just pass 0 as a placeholder here. But, yes, passing domid instead
> keeps the right domain status. I'll change it in the next version.

Thanks. This is much less error prone to future changes.

Christoph

> 
>>
>>>  }
>>>
>>>  static int inject_mci_status(xc_interface *xc_handle,
>>> @@ -383,7 +296,7 @@ static int inject_mci_status(xc_interface *xc_handle,
>>>                               uint64_t val)
>>>  {
>>>      return add_msr_bank_intpose(xc_handle, cpu_nr, MC_MSRINJ_F_INTERPOSE,
>>> -                                    MCi_type_STATUS, bank, val);
>>> +                                    MCi_type_STATUS, bank, val, 0);
>>
>> Why do you hardcode 0 here?
>> I think you should pass domid in here so that the DomU sees the right
>> status value.
>>
> 
> the same as above
> 
>>>  }
>>>
>>>  static int inject_mci_misc(xc_interface *xc_handle,
>>> @@ -392,22 +305,23 @@ static int inject_mci_misc(xc_interface *xc_handle,
>>>                               uint64_t val)
>>>  {
>>>      return add_msr_bank_intpose(xc_handle, cpu_nr, MC_MSRINJ_F_INTERPOSE,
>>> -                                    MCi_type_MISC, bank, val);
>>> +                                    MCi_type_MISC, bank, val, 0);
>>>  }
>>>
>>>  static int inject_mci_addr(xc_interface *xc_handle,
>>>                               uint32_t cpu_nr,
>>>                               uint64_t bank,
>>> -                             uint64_t val)
>>> +                             uint64_t val,
>>> +                             domid_t domid)
>>>  {
>>> -    return add_msr_bank_intpose(xc_handle, cpu_nr, MC_MSRINJ_F_INTERPOSE,
>>> -                                    MCi_type_ADDR, bank, val);
>>> +    return add_msr_bank_intpose(xc_handle, cpu_nr,
>>> +                                    MC_MSRINJ_F_INTERPOSE | MC_MSRINJ_F_GPADDR,
>>> +                                    MCi_type_ADDR, bank, val, domid);
>>>  }
>>>
>>>  static int inject(xc_interface *xc_handle, struct mce_info *mce,
>>>                    uint32_t cpu_nr, uint32_t domain, uint64_t gaddr)
>>>  {
>>> -    uint64_t gpfn, mfn, haddr;
>>>      int ret = 0;
>>>
>>>      ret = inject_mcg_status(xc_handle, cpu_nr, mce->mcg_stat);
>>> @@ -424,12 +338,7 @@ static int inject(xc_interface *xc_handle, struct mce_info *mce,
>>>      if ( ret )
>>>          err(xc_handle, "Failed to inject MCi_MISC MSR");
>>>
>>> -    gpfn = gaddr >> PAGE_SHIFT;
>>> -    mfn = mca_gpfn_to_mfn(xc_handle, domain, gpfn);
>>> -    if (!mfn_valid(mfn))
>>> -        err(xc_handle, "The MFN is not valid");
>>> -    haddr = (mfn << PAGE_SHIFT) | (gaddr & (PAGE_SIZE - 1));
>>> -    ret = inject_mci_addr(xc_handle, cpu_nr, mce->bank, haddr);
>>> +    ret = inject_mci_addr(xc_handle, cpu_nr, mce->bank, gaddr, domain);
>>>      if ( ret )
>>>          err(xc_handle, "Failed to inject MCi_ADDR MSR");
>>>
>>> @@ -507,7 +416,7 @@ int main(int argc, char *argv[])
>>>      uint32_t domid;
>>>      xc_interface *xc_handle;
>>>      int cpu_nr;
>>> -    int64_t gaddr, gpfn, mfn, haddr, max_gpa;
>>> +    uint64_t gaddr, max_gpa;
>>>
>>>      /* Default Value */
>>>      domid = DOMID_XEN;
>>> @@ -563,16 +472,10 @@ int main(int argc, char *argv[])
>>>      Lprintf("get gaddr of error inject is: 0x%lx", gaddr);
>>>
>>>      if ( dump ) {
>>> -        gpfn = gaddr >> PAGE_SHIFT;
>>> -        mfn = mca_gpfn_to_mfn(xc_handle, domid, gpfn);
>>> -        if (!mfn_valid(mfn))
>>> -            err(xc_handle, "The MFN is not valid");
>>> -        haddr = (mfn << PAGE_SHIFT) | (gaddr & (PAGE_SIZE - 1));
>>>          if ( domid == DOMID_XEN )
>>> -            Lprintf("Xen: mfn=0x%lx, haddr=0x%lx", mfn, haddr);
>>> +            Lprintf("Xen: gaddr=0x%lx", gaddr);
>>>          else
>>> -            Lprintf("Dom%d: gaddr=0x%lx, gpfn=0x%lx, mfn=0x%lx, haddr=0x%lx",
>>> -                    domid, gaddr, gpfn, mfn, haddr);
>>> +            Lprintf("Dom%d: gaddr=0x%lx", domid, gaddr);
>>>          goto out;
>>>      }
>>>
>>>

Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B