Re: [Xen-devel] [PATCH v1] x86/mm: Supresses vm_events caused by page-walks

[Tamas K Lengyel]

On Mon, Oct 30, 2017 at 11:19 AM, Razvan Cojocaru
<rcojoc...@bitdefender.com> wrote:
> On 10/30/2017 07:07 PM, Tamas K Lengyel wrote:
>> On Mon, Oct 30, 2017 at 11:01 AM, Razvan Cojocaru
>> <rcojoc...@bitdefender.com> wrote:
>>> On 10/30/2017 06:39 PM, Tamas K Lengyel wrote:
>>>> On Mon, Oct 30, 2017 at 10:24 AM, Razvan Cojocaru
>>>> <rcojoc...@bitdefender.com> wrote:
>>>>> On 30.10.2017 18:01, Tamas K Lengyel wrote:
>>>>>> On Mon, Oct 30, 2017 at 4:32 AM, Alexandru Isaila
>>>>>> <aisa...@bitdefender.com> wrote:
>>>>>>> This patch is adding a way to enable/disable nested pagefault
>>>>>>> events. It introduces the xc_monitor_nested_pagefault function
>>>>>>> and adds the nested_pagefault_disabled in the monitor structure.
>>>>>>> This is needed by the introspection so it will only get gla
>>>>>>> faults and not get spammed with other faults.
>>>>>>> In p2m_set_ad_bits the v->arch.sse_pg_dirty.eip and
>>>>>>> v->arch.sse_pg_dirty.gla are used to mark that this is the
>>>>>>> second time a fault occurs and the dirty bit is set.
>>>>>>
>>>>>> Could you describe under what conditions do you get these other faults?
>>>>>
>>>>> Hey Tamas, the whole story is at page 8 of this document:
>>>>>
>>>>> https://www.researchgate.net/publication/281835515_Proposed_Processor_Extensions_for_Significant_Speedup_of_Hypervisor_Memory_Introspection
>>>>
>>>> Hi Razvan,
>>>> thanks but I'm not sure that doc addresses my question. You
>>>> effectively filter out npfec_kind_in_gpt and npfec_kind_unknown in
>>>> this patch. The first, npfec_kind_in_gpt should only happen if you
>>>> have restricted access to the gpt with ept and the processor couldn't
>>>> walk the table. But if you don't want to get events of these types
>>>> then why not simply not restrict access the gpt to begin with? And as
>>>> for npfec_kind_unknown, I don't think that gets generated under any
>>>> situation. So hence my question, what is your setup that makes this
>>>> patch necessary?
>>>
>>> On the npfec_kind_unknown case, indeed, we were wondering when that
>>> might possibly occur when discussing this patch - it's probably reserved
>>> for the future?
>>>
>>> On why our introspection engine decides to restrict access to those
>>> specific pages, I am not intimate with its inner workings, and not sure
>>> how much could be disclosed here in any case. Is it not a worthwhile
>>> (and otherwise harmless) tool to be able to switch A/D bits-triggered
>>> EPT faults anyway, for introspection purposes?
>>
>> It changes the default behavior of mem_access events so I just wanted
>> to get some background on when that is really required. Technically
>> there is no reason why we couldn't do that filtering in Xen. I think
>> it might be better to flip the filter the other way though so the
>> default behavior remains as is (ie. change the option to enable
>> filtering instead of enabling monitoring).
>
> Wait, it shouldn't change the default behaviour at all. If nobody calls
> that function, all the EPT event kinds should be sent out - the new
> monitor flag is a "disable" flag for non-GLA event (the so-called
> "nested page fault" events).

Oh yea you are right, I completely overlooked that it is named
"nested_pagefault_disabled" =) Maybe a comment in the domctl header
would be warranted to note that this is enabled by default when
mem_access is used.

Tamas

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

Re: [Xen-devel] [PATCH v1] x86/mm: Supresses vm_events caused by page-walks

[Tamas K Lengyel]

On Mon, Oct 30, 2017 at 11:01 AM, Razvan Cojocaru
<rcojoc...@bitdefender.com> wrote:
> On 10/30/2017 06:39 PM, Tamas K Lengyel wrote:
>> On Mon, Oct 30, 2017 at 10:24 AM, Razvan Cojocaru
>> <rcojoc...@bitdefender.com> wrote:
>>> On 30.10.2017 18:01, Tamas K Lengyel wrote:
>>>> On Mon, Oct 30, 2017 at 4:32 AM, Alexandru Isaila
>>>> <aisa...@bitdefender.com> wrote:
>>>>> This patch is adding a way to enable/disable nested pagefault
>>>>> events. It introduces the xc_monitor_nested_pagefault function
>>>>> and adds the nested_pagefault_disabled in the monitor structure.
>>>>> This is needed by the introspection so it will only get gla
>>>>> faults and not get spammed with other faults.
>>>>> In p2m_set_ad_bits the v->arch.sse_pg_dirty.eip and
>>>>> v->arch.sse_pg_dirty.gla are used to mark that this is the
>>>>> second time a fault occurs and the dirty bit is set.
>>>>
>>>> Could you describe under what conditions do you get these other faults?
>>>
>>> Hey Tamas, the whole story is at page 8 of this document:
>>>
>>> https://www.researchgate.net/publication/281835515_Proposed_Processor_Extensions_for_Significant_Speedup_of_Hypervisor_Memory_Introspection
>>
>> Hi Razvan,
>> thanks but I'm not sure that doc addresses my question. You
>> effectively filter out npfec_kind_in_gpt and npfec_kind_unknown in
>> this patch. The first, npfec_kind_in_gpt should only happen if you
>> have restricted access to the gpt with ept and the processor couldn't
>> walk the table. But if you don't want to get events of these types
>> then why not simply not restrict access the gpt to begin with? And as
>> for npfec_kind_unknown, I don't think that gets generated under any
>> situation. So hence my question, what is your setup that makes this
>> patch necessary?
>
> On the npfec_kind_unknown case, indeed, we were wondering when that
> might possibly occur when discussing this patch - it's probably reserved
> for the future?
>
> On why our introspection engine decides to restrict access to those
> specific pages, I am not intimate with its inner workings, and not sure
> how much could be disclosed here in any case. Is it not a worthwhile
> (and otherwise harmless) tool to be able to switch A/D bits-triggered
> EPT faults anyway, for introspection purposes?

It changes the default behavior of mem_access events so I just wanted
to get some background on when that is really required. Technically
there is no reason why we couldn't do that filtering in Xen. I think
it might be better to flip the filter the other way though so the
default behavior remains as is (ie. change the option to enable
filtering instead of enabling monitoring).

Tamas

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

Re: [Xen-devel] [PATCH v1] x86/mm: Supresses vm_events caused by page-walks

[Tamas K Lengyel]

On Mon, Oct 30, 2017 at 10:24 AM, Razvan Cojocaru
<rcojoc...@bitdefender.com> wrote:
> On 30.10.2017 18:01, Tamas K Lengyel wrote:
>> On Mon, Oct 30, 2017 at 4:32 AM, Alexandru Isaila
>> <aisa...@bitdefender.com> wrote:
>>> This patch is adding a way to enable/disable nested pagefault
>>> events. It introduces the xc_monitor_nested_pagefault function
>>> and adds the nested_pagefault_disabled in the monitor structure.
>>> This is needed by the introspection so it will only get gla
>>> faults and not get spammed with other faults.
>>> In p2m_set_ad_bits the v->arch.sse_pg_dirty.eip and
>>> v->arch.sse_pg_dirty.gla are used to mark that this is the
>>> second time a fault occurs and the dirty bit is set.
>>
>> Could you describe under what conditions do you get these other faults?
>
> Hey Tamas, the whole story is at page 8 of this document:
>
> https://www.researchgate.net/publication/281835515_Proposed_Processor_Extensions_for_Significant_Speedup_of_Hypervisor_Memory_Introspection

Hi Razvan,
thanks but I'm not sure that doc addresses my question. You
effectively filter out npfec_kind_in_gpt and npfec_kind_unknown in
this patch. The first, npfec_kind_in_gpt should only happen if you
have restricted access to the gpt with ept and the processor couldn't
walk the table. But if you don't want to get events of these types
then why not simply not restrict access the gpt to begin with? And as
for npfec_kind_unknown, I don't think that gets generated under any
situation. So hence my question, what is your setup that makes this
patch necessary?

Thanks,
Tamas

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

Re: [Xen-devel] [PATCH v1] x86/mm: Supresses vm_events caused by page-walks

[Tamas K Lengyel]

On Mon, Oct 30, 2017 at 4:32 AM, Alexandru Isaila
 wrote:
> This patch is adding a way to enable/disable nested pagefault
> events. It introduces the xc_monitor_nested_pagefault function
> and adds the nested_pagefault_disabled in the monitor structure.
> This is needed by the introspection so it will only get gla
> faults and not get spammed with other faults.
> In p2m_set_ad_bits the v->arch.sse_pg_dirty.eip and
> v->arch.sse_pg_dirty.gla are used to mark that this is the
> second time a fault occurs and the dirty bit is set.

Could you describe under what conditions do you get these other faults?

Thanks,
Tamas

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

Re: [Xen-devel] Booting signed xen.efi through shim

[Tamas K Lengyel]

On Fri, Sep 22, 2017 at 5:11 PM, Daniel Kiper  wrote:
> On Fri, Sep 22, 2017 at 02:25:46AM -0600, Jan Beulich wrote:
>> >>> On 22.09.17 at 00:46,  wrote:
>> > One piece that I see still missing is the Xen command line parameters
>> > not being verified. It would be ideal to have the option to get that
>> > set during compile time as well, similar to Linux's CONFIG_CMDLINE
>> > option, to avoid for example getting iommu or XSM being turned off by
>> > someone with physical access.
>>
>> We do have CMDLINE and CMDLINE_OVERRIDE. But for someone
>> with physical access it would likely also be possible to avoid secure
>> boot altogether?
>
> Another solutions is here: 
> http://lists.gnu.org/archive/html/grub-devel/2017-07/msg3.html
> It is TPM based and WIP. It requires verifiers framework which should
> be posted on grub-devel soon. Or you can add your own method based
> on verifiers. Patches are welcome...
>
> Have a nice weekend,
>
> Daniel

There is an additional problem with Xen.efi being measured into TPM2
devices through the shim. The shim uses the PE_COFF_IMAGE flag when
calling TPM2's HashLogExtendEvent function. At least on my Dell
ultrabook this causes the TPM to return EFI_UNSUPPORTED error, which
according to the spec means "If the Flags bitmap has the PE_COFF_IMAGE
bit SET but the PE/COFF image is corrupt or not understood the
function shall return EFI_UNSUPPORTED". As by default the shim ignores
TPM errors (yikes!) and the verification step works, xen can
successfully boot afterwards, but AFAICT without a measurement being
stored in TPM2. At the moment unfortunately I have no idea why TPM2
have a problem interpreting Xen.efi properly. For now an easy "fix" is
to just have the shim call without PE_COFF_IMAGE flag. If anyone else
has a TPM2 device, it might be worthwhile double-checking whether it's
just a problem with my specific TPM or if it's a problem with
Xen.efi's PE/COFF header.

Tamas

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

Re: [Xen-devel] [PATCH RFC 05/14] xen: vmx: Disable the 2M/1G superpage when SPP enabled

[Tamas K Lengyel]

On Wed, Oct 25, 2017 at 9:32 AM, Yi Zhang <yi.z.zh...@linux.intel.com> wrote:
> On 2017-10-24 at 11:43:45 -0600, Tamas K Lengyel wrote:
>> On Fri, Oct 20, 2017 at 2:44 AM, Yi Zhang <yi.z.zh...@linux.intel.com> wrote:
>> > On 2017-10-19 at 12:17:12 -0600, Tamas K Lengyel wrote:
>> >> On Thu, Oct 19, 2017 at 2:11 AM, Zhang Yi <yi.z.zh...@linux.intel.com> 
>> >> wrote:
>> >> > From: Zhang Yi Z <yi.z.zh...@linux.intel.com>
>> >> >
>> >> > Current we only support Sub-page Protection on the 4k
>> >> > page table.
>> >> >
>> >> > Signed-off-by: Zhang Yi Z <yi.z.zh...@linux.intel.com>
>> >> > ---
>> >> >  xen/arch/x86/hvm/vmx/vmx.c | 6 ++
>> >> >  1 file changed, 6 insertions(+)
>> >> >
>> >> > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>> >> > index 04ae0d6..a4c24bb 100644
>> >> > --- a/xen/arch/x86/hvm/vmx/vmx.c
>> >> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> >> > @@ -2497,6 +2497,12 @@ const struct hvm_function_table * __init 
>> >> > start_vmx(void)
>> >> >  vmx_function_table.get_guest_bndcfgs = vmx_get_guest_bndcfgs;
>> >> >  }
>> >> >
>> >> > +if ( cpu_has_vmx_ept_spp )
>> >>
>> >> I think this really only ought to happen if the command-line option
>> >> has also been enabled.
>> >
>> > Sorry, didn't catch your point, the command line option opt_hap_2m and
>> > opt_hap_1G was enable by default, I need to  disable the supper page
>> > when spp feature enabled. Did you mean that if we enable 2M/1G by
>> > command-line we couldn't disable it here? yes, it is, I will improve
>> > this logic. Thank you Tamas.
>>
>> I meant that right now "cpu_has_vmx_ept_spp" looks like just checks
>> whether the CPU supports SPP, not whether the command-line option was
>> set to enable it. If the command line option is not set (or
>> specifically disables SPP) then the large pages shouldn't get
>> disabled.
>>
>
> In patch 02/14, if we didn't set spp_enable, we will not set the spp cap
> in vmx_secondary_exec_control, so cp_has_vmx_ept_spp flag will set when
> hardware has spp cap and xen cmdlline passed the parameter "spp_enable=1"

OK, thanks, I missed that.

>
>> >
>> >>
>> >> > +{
>> >> > +vmx_function_table.hap_capabilities &= ~HVM_HAP_SUPERPAGE_2MB;
>> >> > +vmx_function_table.hap_capabilities &= ~HVM_HAP_SUPERPAGE_1GB;
>> >> > +}
>> >> > +
>> >> >  setup_vmcs_dump();
>> >> >
>> >> >  lbr_tsx_fixup_check();
>> >> > --
>> >> > 2.7.4

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

Re: [Xen-devel] [PATCH RFC 05/14] xen: vmx: Disable the 2M/1G superpage when SPP enabled

[Tamas K Lengyel]

On Fri, Oct 20, 2017 at 2:44 AM, Yi Zhang <yi.z.zh...@linux.intel.com> wrote:
> On 2017-10-19 at 12:17:12 -0600, Tamas K Lengyel wrote:
>> On Thu, Oct 19, 2017 at 2:11 AM, Zhang Yi <yi.z.zh...@linux.intel.com> wrote:
>> > From: Zhang Yi Z <yi.z.zh...@linux.intel.com>
>> >
>> > Current we only support Sub-page Protection on the 4k
>> > page table.
>> >
>> > Signed-off-by: Zhang Yi Z <yi.z.zh...@linux.intel.com>
>> > ---
>> >  xen/arch/x86/hvm/vmx/vmx.c | 6 ++
>> >  1 file changed, 6 insertions(+)
>> >
>> > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>> > index 04ae0d6..a4c24bb 100644
>> > --- a/xen/arch/x86/hvm/vmx/vmx.c
>> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> > @@ -2497,6 +2497,12 @@ const struct hvm_function_table * __init 
>> > start_vmx(void)
>> >  vmx_function_table.get_guest_bndcfgs = vmx_get_guest_bndcfgs;
>> >  }
>> >
>> > +if ( cpu_has_vmx_ept_spp )
>>
>> I think this really only ought to happen if the command-line option
>> has also been enabled.
>
> Sorry, didn't catch your point, the command line option opt_hap_2m and
> opt_hap_1G was enable by default, I need to  disable the supper page
> when spp feature enabled. Did you mean that if we enable 2M/1G by
> command-line we couldn't disable it here? yes, it is, I will improve
> this logic. Thank you Tamas.

I meant that right now "cpu_has_vmx_ept_spp" looks like just checks
whether the CPU supports SPP, not whether the command-line option was
set to enable it. If the command line option is not set (or
specifically disables SPP) then the large pages shouldn't get
disabled.

>
>>
>> > +{
>> > +vmx_function_table.hap_capabilities &= ~HVM_HAP_SUPERPAGE_2MB;
>> > +vmx_function_table.hap_capabilities &= ~HVM_HAP_SUPERPAGE_1GB;
>> > +}
>> > +
>> >  setup_vmcs_dump();
>> >
>> >  lbr_tsx_fixup_check();
>> > --
>> > 2.7.4

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

Re: [Xen-devel] [RFC 0/4] TEE mediator framework + OP-TEE mediator

[Tamas K Lengyel]

>> In previous discussion we considered only two variants: in XEN or outside
>> XEN. Stubdomain approach looks more secure, but I'm not sure that it is
>> true.
>> Such stubdomain will need access to all guests memory. If you managed to
>> gain control on mediator stubdomain, you can do anything you want with all
>> guests.
>
>
> That's slightly untrue. The stubdomain will only be able to mess with
> domains using TEE.

Would it be feasible to have multiple TEE stubdoms providing the
interface for select domUs (with XSM)? IMHO that would provide the
greatest disaggregation and thus the most security.

Tamas

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

Re: [Xen-devel] [PATCH RFC 09/14] xen: vmx: Introduce a Hyper call to set subpage

[Tamas K Lengyel]

On Thu, Oct 19, 2017 at 2:13 AM, Zhang Yi  wrote:
> From: Zhang Yi Z 
>
> The Hypercall is defined as HVMOP_set_subpage

Are there any expected use-cases where a HVM guest would need access
to this hypercall? Is spp compatible with #VE? If not, I think it
would be better to integrate this with the existing xen_mem_access_op.

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

Re: [Xen-devel] [PATCH RFC 08/14] xen: vmx: Added setup spp page structure.

[Tamas K Lengyel]

On Thu, Oct 19, 2017 at 2:12 AM, Zhang Yi  wrote:
> From: Zhang Yi Z 
>
> The hardware uses the guest-physical address and bits 11:7 of the
> address accessed to lookup the SPPT to fetch a write permission bit for
> the 128 byte wide sub-page region being accessed within the 4K
> guest-physical page. If the sub-page region write permission bit is set,
> the write is allowed; otherwise the write is disallowed and results in
> an EPT violation.
>
> Guest-physical pages mapped via leaf EPT-paging-structures for which the
> accumulated write-access bit and the SPP bits are both clear (0)
> generate
> EPT violations on memory writes accesses. Guest-physical pages mapped
> via EPT-paging-structure for which the accumulated write-access bit is
> set (1) allow writes, effectively ignoring the SPP bit on the leaf
> EPT-paging structure.
>
> Software will setup the spp page table level4,3,2 as well as EPT page
> structure, and fill the level1 via the 32 bit bitmap per a single 4K
> page.
> Now it could be divided to 32 x 128 sub-pages.
>
> Signed-off-by: Zhang Yi Z 
> ---
>  xen/arch/x86/mm/mem_access.c  | 35 +++
>  xen/arch/x86/mm/p2m-ept.c | 94 
> +++
>  xen/include/asm-x86/hvm/vmx/vmx.h | 10 +
>  xen/include/asm-x86/p2m.h |  3 ++
>  4 files changed, 142 insertions(+)
>
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index a471c74..1b97469 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -490,6 +490,41 @@ unlock_exit:
>  return rc;
>  }
>
> +static u64 format_spp_spte(u32 spp_wp_bitmap)
> +{
> +   u64 new_spte = 0;
> +   int i = 0;
> +
> +   /*
> +* One 4K page contains 32 sub-pages, in SPP table L4E, old bits
> +* are reserved, so we need to transfer u32 subpage write
> +* protect bitmap to u64 SPP L4E format.
> +*/
> +   while ( i < 32 ) {
> +   if ( spp_wp_bitmap & (1ULL << i) )
> +   new_spte |= 1ULL << (i * 2);
> +
> +   i++;
> +   }
> +
> +   return new_spte;
> +}
> +
> +int p2m_set_spp_page_st(struct domain *d, gfn_t gfn, uint32_t access_map)

So nothing in this patch makes use of this function. Could you please
re-organize the patchset so this is included with the patch that
starts using it?

> +{
> +struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +u64 access = format_spp_spte(access_map);
> +unsigned long gfn_l = gfn_x(gfn);
> +int ret = -1;
> +
> +p2m_lock(p2m);
> +if ( p2m->spp_set_entry )
> +ret = p2m->spp_set_entry(p2m, gfn_l, access);
> +p2m_unlock(p2m);
> +
> +return ret;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C

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

Re: [Xen-devel] [PATCH RFC 05/14] xen: vmx: Disable the 2M/1G superpage when SPP enabled

[Tamas K Lengyel]

On Thu, Oct 19, 2017 at 2:11 AM, Zhang Yi  wrote:
> From: Zhang Yi Z 
>
> Current we only support Sub-page Protection on the 4k
> page table.
>
> Signed-off-by: Zhang Yi Z 
> ---
>  xen/arch/x86/hvm/vmx/vmx.c | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 04ae0d6..a4c24bb 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2497,6 +2497,12 @@ const struct hvm_function_table * __init 
> start_vmx(void)
>  vmx_function_table.get_guest_bndcfgs = vmx_get_guest_bndcfgs;
>  }
>
> +if ( cpu_has_vmx_ept_spp )

I think this really only ought to happen if the command-line option
has also been enabled.

> +{
> +vmx_function_table.hap_capabilities &= ~HVM_HAP_SUPERPAGE_2MB;
> +vmx_function_table.hap_capabilities &= ~HVM_HAP_SUPERPAGE_1GB;
> +}
> +
>  setup_vmcs_dump();
>
>  lbr_tsx_fixup_check();
> --
> 2.7.4

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

Re: [Xen-devel] [PATCH v2] x86/hvm: Add MSR old value

[Tamas K Lengyel]

On Fri, Oct 13, 2017 at 9:50 AM, Jan Beulich <jbeul...@suse.com> wrote:
>>>> On 13.10.17 at 14:50, <aisa...@bitdefender.com> wrote:
>> This patch adds the old value param and the onchangeonly option
>> to the VM_EVENT_REASON_MOV_TO_MSR event.
>>
>> The param was added to the vm_event_mov_to_msr struct and to the
>> hvm_monitor_msr function. Finally I've changed the bool_t param
>> to a bool for the hvm_msr_write_intercept function.
>>
>> Signed-off-by: Alexandru Isaila <aisa...@bitdefender.com>
>> Acked-by: Tamas K Lengyel <ta...@tklengyel.com>
>
> I think this should have been dropped with a bug having been fixed
> (for only cosmetic changes it would be fine to keep). For the bits
> where it's applicable
> Acked-by: Jan Beulich <jbeul...@suse.com>

Indeed. It's fine for now, my ack still stands.

Thanks,
Tamas

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

Re: [Xen-devel] [PATCH v1] x86/hvm: Add MSR old value

[Tamas K Lengyel]

On Fri, Oct 13, 2017 at 6:17 AM, Jan Beulich  wrote:
 On 13.10.17 at 12:36,  wrote:
>> On 13.10.2017 13:29, Jan Beulich wrote:
 +__set_bit(index + sizeof(struct monitor_msr_bitmap), bitmap);
>>>
>>> I think you miss "* 8" here - a bit position plus sizeof() doesn't
>>> produce any useful value.
>>>
>>> But what's worse - having read till the end of the patch I don't
>>> see you change any allocation, yet you clearly need to double
>>> the space now that you need two bits per MSR.
>>
>> We did this:
>>
>> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
>> index e59f1f5..a3046c6 100644
>> --- a/xen/arch/x86/monitor.c
>> +++ b/xen/arch/x86/monitor.c
>> @@ -25,7 +25,7 @@
>>   int arch_monitor_init_domain(struct domain *d)
>>   {
>>   if ( !d->arch.monitor.msr_bitmap )
>> -d->arch.monitor.msr_bitmap = xzalloc(struct monitor_msr_bitmap);
>> +d->arch.monitor.msr_bitmap = xzalloc_array(struct 
>> monitor_msr_bitmap, 2);
>>
>>   if ( !d->arch.monitor.msr_bitmap )
>>   return -ENOMEM;
>> @@ -67,7 +67,7 @@ static unsigned long *monitor_bitmap_for_msr(const struct 
>> domain *d, u32 *msr)
>>   }
>>   }
>>
>> I.e., we are now allocating an array of size 2 of struct
>> monitor_msr_bitmaps with xzalloc_array().
>
> Oh, I'm not sure how I could overlook this considering that I
> specifically looked up the allocation point and searched through
> the patch for a respective change. I'm sorry for the noise in
> this regard. I do think though that the chosen model is a little
> odd and fragile, but that's something you and Tamas as the
> maintainers of the code have to judge about.
>

It looks fine to me.

Thanks,
Tamas

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

Re: [Xen-devel] [PATCH v1] x86/hvm: Add MSR old value

[Tamas K Lengyel]

On Thu, Oct 12, 2017 at 3:10 AM, Alexandru Isaila
<aisa...@bitdefender.com> wrote:
> This patch adds the old value param and the onchangeonly option
> to the VM_EVENT_REASON_MOV_TO_MSR event.
>
> The param was added to the vm_event_mov_to_msr struct and to the
> hvm_monitor_msr function. Finally I've changed the bool_t param
> to a bool for the hvm_msr_write_intercept function.
>
> Signed-off-by: Alexandru Isaila <aisa...@bitdefender.com>

Acked-by: Tamas K Lengyel <ta...@tklengyel.com>

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

Re: [Xen-devel] [PATCH v2 3/9] xen/x86: mem_sharing: Use copy_domain_page in __mem_sharing_unshare_page

[Tamas K Lengyel]

On Thu, Oct 5, 2017 at 11:42 AM, Julien Grall <julien.gr...@linaro.org> wrote:
> The function __mem_sharing_unshare_page contains an open-code version of
> copy_domain_page. Use the function to simplify a bit the code.
>
> At the same time replace _mfn(__page_to_mfn(...)) by page_to_mfn(...)
> given that the file given already provides a typesafe version of page_to_mfn.
>
> Signed-off-by: Julien Grall <julien.gr...@linaro.org>

Acked-by: Tamas K Lengyel <ta...@tklengyel.com>

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

Re: [Xen-devel] [PATCH 7/7] xen: Convert __page_to_mfn and __mfn_to_page to use typesafe MFN

[Tamas K Lengyel]

On Wed, Oct 4, 2017 at 12:15 PM, Julien Grall <julien.gr...@linaro.org> wrote:
> Most of the users of page_to_mfn and mfn_to_page are either overriding
> the macros to make them work with mfn_t or use mfn_x/_mfn because the
> rest of the function use mfn_t.
>
> So make __page_to_mfn and __mfn_to_page return mfn_t by default.
>
> Only reasonable clean-ups are done in this patch because it is
> already quite big. So some of the files now override page_to_mfn and
> mfn_to_page to avoid using mfn_t.
>
> Signed-off-by: Julien Grall <julien.gr...@linaro.org>

Acked-by: Tamas K Lengyel <ta...@tklengyel.com>

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

Re: [Xen-devel] [PATCH v3 08/15] xen/x86: p2m: Use typesafe gfn for the P2M callbacks get_entry and set_entry

[Tamas K Lengyel]

On Mon, Oct 2, 2017 at 6:59 AM, Julien Grall <julien.gr...@arm.com> wrote:
> Signed-off-by: Julien Grall <julien.gr...@arm.com>
> Acked-by: Andrew Cooper <andrew.coop...@citrix.com>
> Reviewed-by: Kevin Tian <kevin.t...@intel.com>
> Acked-by: Razvan Cojocaru <rcojoc...@bitdefender.com>
> Reviewed-by: Wei Liu <wei.l...@citrix.com>

Acked-by: Tamas K Lengyel <ta...@tklengyel.com>

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

Re: [Xen-devel] Booting signed xen.efi through shim

[Tamas K Lengyel]

On Fri, Sep 22, 2017 at 2:25 AM, Jan Beulich  wrote:
 On 22.09.17 at 00:46,  wrote:
>> One piece that I see still missing is the Xen command line parameters
>> not being verified. It would be ideal to have the option to get that
>> set during compile time as well, similar to Linux's CONFIG_CMDLINE
>> option, to avoid for example getting iommu or XSM being turned off by
>> someone with physical access.
>
> We do have CMDLINE and CMDLINE_OVERRIDE. But for someone
> with physical access it would likely also be possible to avoid secure
> boot altogether?
>

Interesting, it never showed up for me in make menuconfig. Searching
for it does show it but seems to be not accessible in menuconfig.
Anyway, good to know! And indeed, someone having physical access could
do a firmware reset by taking the computer apart (firmware would need
to be password protected if Secureboot is enabled). What I meant is
protection against someone during boot changing the config options or
altering the cfg file on disk.

And even with a firmware reset, I guess it's up to the OEM to decide
what the reset state is. So it might be possible in some situations to
have the reset state also include having Secureboot enabled with the
custom keys. Otherwise having the disk encrypted with the key being
sealed in the TPM against PCR[0-4] for example should work. Provided
of course that a malicious firmware can't fake those measurements
somehow.

Tamas

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

Re: [Xen-devel] Booting signed xen.efi through shim

[Tamas K Lengyel]

On Wed, Sep 20, 2017 at 10:10 AM, Daniel Kiper <daniel.ki...@oracle.com> wrote:
> On Wed, Sep 20, 2017 at 09:59:51AM -0600, Tamas K Lengyel wrote:
>> On Wed, Sep 20, 2017 at 9:46 AM, Jan Beulich <jbeul...@suse.com> wrote:
>> >>>> On 20.09.17 at 17:20, <ta...@tklengyel.com> wrote:
>> >> On Wed, Sep 20, 2017 at 12:30 AM, Jan Beulich <jbeul...@suse.com> wrote:
>> >>>>>> On 20.09.17 at 00:23, <ta...@tklengyel.com> wrote:
>> >>>> Yeap, the shim pretty simply removed the .reloc section as it was
>> >>>> marked discardable and did the relocations for Xen. So with that
>> >>>> removed from the shim I no longer get the error and I see that the
>> >>>> dom0 kernel gets verified using the shim lock protocol.
>> >>>
>> >>> So did you instead try whether simply clearing the discardable
>> >>> flag from the section also helps? The flag ought to matter in
>> >>> paged environments only (which EFI isn't despite paging being
>> >>> enabled), but as we see some people think otherwise.
>> >>
>> >> Yes, that would work. Even if the shim does its relocations everything
>> >> "just works" as long as Xen can also find the .reloc section. For now
>> >> it was just simpler for me to patch the shim to copy the reloc section
>> >> but it would be ideal if the xen.efi that's produced during
>> >> compilation would not have that flag set to begin with. I did search
>> >> around briefly to see where that flag is coming from but the only
>> >> reference to it within Xen I found was arch/x86/efi/mkreloc.c. So sans
>> >> writing a separate tool that binary patches xen.efi after compilation
>> >> to remove that flag I'm not sure how to get that done.
>> >
>> > That'll likely be another binutils tweak. What I find odd is that you're
>> > apparently the only one to have this problem.
>>
>> My guess is that not many people have actually tried booting Xen
>> through the shim before. I looked through the shim history and the
>> reloc section was discarded all the way back to several years if it
>> was marked discardable. So unless the discardable flag is something
>> that only has been added to the reloc section recently, someone should
>> have run into it already.
>
> I have played with Xen master in July and have not seen any issues.
> I will take a stab at it probably next week.
>
>> >>>> I still didn't
>> >>>> get dom0 to boot for some reason but that might be an unrelated issue
>> >>>> (and I have no serial console right now). Nevertheless, progress!
>> >>>
>> >>> And it doesn't get far enough for you to see any output at all?
>> >>> Did you try "earlyprintk=xen" on the kernel command line and/or
>> >>> "vga=keep" on the Xen one?
>> >>
>> >> I tried with both just now, no output at all from Xen on the screen
>> >> after it exits EFI boot. I also couldn't get any output from it on my
>> >> other laptop with Intel AMT.
>> >
>> > Odd.
>> >
>> >> I did manage to get another Linux kernel booting but my goal was to
>> >> get a shim verified dom0 kernel booting without an initrd image as
>> >> right now the ramdisk is not verified by the shim (also not sure how
>> >> that's supposed to work as sbsign/pesign can only deal with PE/COFF
>> >> binaries which the ramdisk isn't).
>> >
>> > That's not how it's supposed to work, I think. Just like the shim only
>> > verifies Xen and hands through the other modules unchecked, Xen
>> > only verifies the Dom0 kernel image (with the help of the shim). It's
>> > the Dom0 kernel to then verify the content (not necessarily the
>> > blob) of the initrd.
>> >
>>
>> Yea, that would be a sensible approach though I haven't (yet) found
>> anything that I could use with linux to do that initrd verification.
>> So my approach right now is to get the initrd baked into the dom0
>> kernel, that way the whole thing can be signed and Xen can verify both
>> in one shot using shim.
>
> Partial solution for your problem is Linux kernel module signing.
>

(Sorry for the previous empty one).

I've managed to get initrd and the cmdline baked into linux and get
the whole thing verified by the shim. It's not perfect because now
updates are a bit more cumbersome to deliver but at least it works.
Linux kernel module signing would be a good way forward from here to
get the modules verified as well. Or the necessary modules could also
be baked into the kernel if known at compile time.

One piece that I see still missing is the Xen command line parameters
not being verified. It would be ideal to have the option to get that
set during compile time as well, similar to Linux's CONFIG_CMDLINE
option, to avoid for example getting iommu or XSM being turned off by
someone with physical access.

Tamas

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

Re: [Xen-devel] Booting signed xen.efi through shim

[Tamas K Lengyel]

On Wed, Sep 20, 2017 at 10:10 AM, Daniel Kiper <daniel.ki...@oracle.com> wrote:
> On Wed, Sep 20, 2017 at 09:59:51AM -0600, Tamas K Lengyel wrote:
>> On Wed, Sep 20, 2017 at 9:46 AM, Jan Beulich <jbeul...@suse.com> wrote:
>> >>>> On 20.09.17 at 17:20, <ta...@tklengyel.com> wrote:
>> >> On Wed, Sep 20, 2017 at 12:30 AM, Jan Beulich <jbeul...@suse.com> wrote:
>> >>>>>> On 20.09.17 at 00:23, <ta...@tklengyel.com> wrote:
>> >>>> Yeap, the shim pretty simply removed the .reloc section as it was
>> >>>> marked discardable and did the relocations for Xen. So with that
>> >>>> removed from the shim I no longer get the error and I see that the
>> >>>> dom0 kernel gets verified using the shim lock protocol.
>> >>>
>> >>> So did you instead try whether simply clearing the discardable
>> >>> flag from the section also helps? The flag ought to matter in
>> >>> paged environments only (which EFI isn't despite paging being
>> >>> enabled), but as we see some people think otherwise.
>> >>
>> >> Yes, that would work. Even if the shim does its relocations everything
>> >> "just works" as long as Xen can also find the .reloc section. For now
>> >> it was just simpler for me to patch the shim to copy the reloc section
>> >> but it would be ideal if the xen.efi that's produced during
>> >> compilation would not have that flag set to begin with. I did search
>> >> around briefly to see where that flag is coming from but the only
>> >> reference to it within Xen I found was arch/x86/efi/mkreloc.c. So sans
>> >> writing a separate tool that binary patches xen.efi after compilation
>> >> to remove that flag I'm not sure how to get that done.
>> >
>> > That'll likely be another binutils tweak. What I find odd is that you're
>> > apparently the only one to have this problem.
>>
>> My guess is that not many people have actually tried booting Xen
>> through the shim before. I looked through the shim history and the
>> reloc section was discarded all the way back to several years if it
>> was marked discardable. So unless the discardable flag is something
>> that only has been added to the reloc section recently, someone should
>> have run into it already.
>
> I have played with Xen master in July and have not seen any issues.
> I will take a stab at it probably next week.
>
>> >>>> I still didn't
>> >>>> get dom0 to boot for some reason but that might be an unrelated issue
>> >>>> (and I have no serial console right now). Nevertheless, progress!
>> >>>
>> >>> And it doesn't get far enough for you to see any output at all?
>> >>> Did you try "earlyprintk=xen" on the kernel command line and/or
>> >>> "vga=keep" on the Xen one?
>> >>
>> >> I tried with both just now, no output at all from Xen on the screen
>> >> after it exits EFI boot. I also couldn't get any output from it on my
>> >> other laptop with Intel AMT.
>> >
>> > Odd.
>> >
>> >> I did manage to get another Linux kernel booting but my goal was to
>> >> get a shim verified dom0 kernel booting without an initrd image as
>> >> right now the ramdisk is not verified by the shim (also not sure how
>> >> that's supposed to work as sbsign/pesign can only deal with PE/COFF
>> >> binaries which the ramdisk isn't).
>> >
>> > That's not how it's supposed to work, I think. Just like the shim only
>> > verifies Xen and hands through the other modules unchecked, Xen
>> > only verifies the Dom0 kernel image (with the help of the shim). It's
>> > the Dom0 kernel to then verify the content (not necessarily the
>> > blob) of the initrd.
>> >
>>
>> Yea, that would be a sensible approach though I haven't (yet) found
>> anything that I could use with linux to do that initrd verification.
>> So my approach right now is to get the initrd baked into the dom0
>> kernel, that way the whole thing can be signed and Xen can verify both
>> in one shot using shim.
>
> Partial solution for your problem is Linux kernel module signing.
>
> Daniel

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

Re: [Xen-devel] [PATCH 1/2] common/efi: bail if dom0 fails the shim verification step

[Tamas K Lengyel]

On Thu, Sep 21, 2017 at 7:03 AM, Jan Beulich  wrote:
 On 20.09.17 at 22:57,  wrote:
>> --- a/xen/common/efi/boot.c
>> +++ b/xen/common/efi/boot.c
>> @@ -1226,9 +1226,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
>> *SystemTable)
>>  efi_bs->FreePool(name.w);
>>
>>  if ( !EFI_ERROR(efi_bs->LocateProtocol(_lock_guid, NULL,
>> -(void **)_lock)) &&
>> - (status = shim_lock->Verify(kernel.ptr, kernel.size)) != 
>> EFI_SUCCESS )
>> -PrintErrMesg(L"Dom0 kernel image could not be verified", 
>> status);
>> +(void **)_lock)))
>> +{
>> +if  ( shim_lock->Verify(kernel.ptr, kernel.size) != EFI_SUCCESS 
>> )
>> +blexit(L"Dom0 kernel image could not be verified by the 
>> shim.");
>> +
>> +PrintStr(L"Dom0 kernel image was verified by the shim.\r\n");
>> +}
>
> So what is the actual behavioral change you're trying to
> accomplish? PrintErrMesg() already calls blexit(),

Indeed, I've somehow missed that. Sorry for the noise.

Tamas

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

[Xen-devel] [PATCH 2/2] common/efi: give people some time to read messages when debugging

[Tamas K Lengyel]

From: Tamas K Lengyel <lengy...@ainfosec.com>

The EFI messages flash by so fast that it is impossible to catch them without
a serial debugger attached. Sometimes though we don't have that available so
having some time to read the messages off the screen is valuable.

Signed-off-by: Tamas K Lengyel <lengy...@ainfosec.com>
Cc: Jan Beulich <jbeul...@suse.com>
---
 xen/common/efi/boot.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index a3a439b838..1bce148bd9 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -321,6 +321,11 @@ static void __init noreturn blexit(const CHAR16 *str)
 PrintStr((CHAR16 *)str);
 PrintStr(newline);
 
+#ifndef NDEBUG
+if ( efi_bs )
+efi_bs->Stall(500);
+#endif
+
 if ( !efi_bs )
 efi_arch_halt();
 
@@ -1300,6 +1305,10 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
*SystemTable)
 if ( gop )
 efi_set_gop_mode(gop, gop_mode);
 
+#ifndef NDEBUG
+efi_bs->Stall(500);
+#endif
+
 efi_exit_boot(ImageHandle, SystemTable);
 
 efi_arch_post_exit_boot();
-- 
2.11.0


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

[Xen-devel] [PATCH 1/2] common/efi: bail if dom0 fails the shim verification step

[Tamas K Lengyel]

From: Tamas K Lengyel <lengy...@ainfosec.com>

If the shim protocol is located it is expected that the dom0 kernel image
will also pass the shim verification.

Signed-off-by: Tamas K Lengyel <lengy...@ainfosec.com>
Cc: Jan Beulich <jbeul...@suse.com>
---
 xen/common/efi/boot.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 01d33004e0..a3a439b838 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1226,9 +1226,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
*SystemTable)
 efi_bs->FreePool(name.w);
 
 if ( !EFI_ERROR(efi_bs->LocateProtocol(_lock_guid, NULL,
-(void **)_lock)) &&
- (status = shim_lock->Verify(kernel.ptr, kernel.size)) != 
EFI_SUCCESS )
-PrintErrMesg(L"Dom0 kernel image could not be verified", status);
+(void **)_lock)))
+{
+if  ( shim_lock->Verify(kernel.ptr, kernel.size) != EFI_SUCCESS )
+blexit(L"Dom0 kernel image could not be verified by the 
shim.");
+
+PrintStr(L"Dom0 kernel image was verified by the shim.\r\n");
+}
 
 name.s = get_value(, section.s, "ramdisk");
 if ( name.s )
-- 
2.11.0


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

Re: [Xen-devel] Booting signed xen.efi through shim

[Tamas K Lengyel]

On Wed, Sep 20, 2017 at 9:46 AM, Jan Beulich  wrote:
 On 20.09.17 at 17:20,  wrote:
>> On Wed, Sep 20, 2017 at 12:30 AM, Jan Beulich  wrote:
>> On 20.09.17 at 00:23,  wrote:
 Yeap, the shim pretty simply removed the .reloc section as it was
 marked discardable and did the relocations for Xen. So with that
 removed from the shim I no longer get the error and I see that the
 dom0 kernel gets verified using the shim lock protocol.
>>>
>>> So did you instead try whether simply clearing the discardable
>>> flag from the section also helps? The flag ought to matter in
>>> paged environments only (which EFI isn't despite paging being
>>> enabled), but as we see some people think otherwise.
>>
>> Yes, that would work. Even if the shim does its relocations everything
>> "just works" as long as Xen can also find the .reloc section. For now
>> it was just simpler for me to patch the shim to copy the reloc section
>> but it would be ideal if the xen.efi that's produced during
>> compilation would not have that flag set to begin with. I did search
>> around briefly to see where that flag is coming from but the only
>> reference to it within Xen I found was arch/x86/efi/mkreloc.c. So sans
>> writing a separate tool that binary patches xen.efi after compilation
>> to remove that flag I'm not sure how to get that done.
>
> That'll likely be another binutils tweak. What I find odd is that you're
> apparently the only one to have this problem.

My guess is that not many people have actually tried booting Xen
through the shim before. I looked through the shim history and the
reloc section was discarded all the way back to several years if it
was marked discardable. So unless the discardable flag is something
that only has been added to the reloc section recently, someone should
have run into it already.

>
 I still didn't
 get dom0 to boot for some reason but that might be an unrelated issue
 (and I have no serial console right now). Nevertheless, progress!
>>>
>>> And it doesn't get far enough for you to see any output at all?
>>> Did you try "earlyprintk=xen" on the kernel command line and/or
>>> "vga=keep" on the Xen one?
>>
>> I tried with both just now, no output at all from Xen on the screen
>> after it exits EFI boot. I also couldn't get any output from it on my
>> other laptop with Intel AMT.
>
> Odd.
>
>> I did manage to get another Linux kernel booting but my goal was to
>> get a shim verified dom0 kernel booting without an initrd image as
>> right now the ramdisk is not verified by the shim (also not sure how
>> that's supposed to work as sbsign/pesign can only deal with PE/COFF
>> binaries which the ramdisk isn't).
>
> That's not how it's supposed to work, I think. Just like the shim only
> verifies Xen and hands through the other modules unchecked, Xen
> only verifies the Dom0 kernel image (with the help of the shim). It's
> the Dom0 kernel to then verify the content (not necessarily the
> blob) of the initrd.
>

Yea, that would be a sensible approach though I haven't (yet) found
anything that I could use with linux to do that initrd verification.
So my approach right now is to get the initrd baked into the dom0
kernel, that way the whole thing can be signed and Xen can verify both
in one shot using shim.

Tamas

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

Re: [Xen-devel] Booting signed xen.efi through shim

[Tamas K Lengyel]

On Wed, Sep 20, 2017 at 12:30 AM, Jan Beulich  wrote:
 On 20.09.17 at 00:23,  wrote:
>> On Mon, Sep 18, 2017 at 2:58 AM, Jan Beulich  wrote:
>> On 14.09.17 at 18:20,  wrote:
 Of course, you can grab them from here:
 https://drive.google.com/drive/folders/0B5duyI9SzNtWaXE0cjM1QzZJbVk?usp=shar
 ing
>>>
>>> So the dumps of the two (using my own tool) are identical except for
>>> the expected difference due to the certificate. In particular neither
>>> image has any strange relocation types afaics, and both have the
>>> sort of unexpected, but also supposedly benign
>>> IMAGE_SCN_LNK_NRELOC_OVFL flag set for .bss. Hence I'm afraid ...
>>>
 I've verified that xen-signed.efi boots with Secureboot enabled when
 booted directly but doesn't boot through the shim.
>>>
>>> ... you'll need to do some debugging in order to figure out what's
>>> going on here. With the above the prime suspect is the shim though,
>>> fiddling with the image after loading it into memory. So perhaps
>>> dumping the .reloc section contents in order to compare it with
>>> what's in the image may be a suitable approach.
>>
>> Yeap, the shim pretty simply removed the .reloc section as it was
>> marked discardable and did the relocations for Xen. So with that
>> removed from the shim I no longer get the error and I see that the
>> dom0 kernel gets verified using the shim lock protocol.
>
> So did you instead try whether simply clearing the discardable
> flag from the section also helps? The flag ought to matter in
> paged environments only (which EFI isn't despite paging being
> enabled), but as we see some people think otherwise.

Yes, that would work. Even if the shim does its relocations everything
"just works" as long as Xen can also find the .reloc section. For now
it was just simpler for me to patch the shim to copy the reloc section
but it would be ideal if the xen.efi that's produced during
compilation would not have that flag set to begin with. I did search
around briefly to see where that flag is coming from but the only
reference to it within Xen I found was arch/x86/efi/mkreloc.c. So sans
writing a separate tool that binary patches xen.efi after compilation
to remove that flag I'm not sure how to get that done.

>
>> I still didn't
>> get dom0 to boot for some reason but that might be an unrelated issue
>> (and I have no serial console right now). Nevertheless, progress!
>
> And it doesn't get far enough for you to see any output at all?
> Did you try "earlyprintk=xen" on the kernel command line and/or
> "vga=keep" on the Xen one?

I tried with both just now, no output at all from Xen on the screen
after it exits EFI boot. I also couldn't get any output from it on my
other laptop with Intel AMT.

I did manage to get another Linux kernel booting but my goal was to
get a shim verified dom0 kernel booting without an initrd image as
right now the ramdisk is not verified by the shim (also not sure how
that's supposed to work as sbsign/pesign can only deal with PE/COFF
binaries which the ramdisk isn't).

Tamas

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

Re: [Xen-devel] [PATCH 1/2] public/domctl: drop unnecessary typedefs and handles

[Tamas K Lengyel]

On Tue, Sep 12, 2017 at 9:08 AM, Jan Beulich <jbeul...@suse.com> wrote:
> By virtue of the struct xen_domctl container structure, most of them
> are really just cluttering the name space.
>
> While doing so,
> - convert an enum typed (pt_irq_type_t) structure field to a fixed
>   width type,
> - make x86's paging_domctl() and descendants take a properly typed
>   handle,
> - add const in a few places.
>
> Signed-off-by: Jan Beulich <jbeul...@suse.com>

Acked-by: Tamas K Lengyel <ta...@tklengyel.com>

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

Re: [Xen-devel] Booting signed xen.efi through shim

[Tamas K Lengyel]

On Mon, Sep 18, 2017 at 2:58 AM, Jan Beulich  wrote:
 On 14.09.17 at 18:20,  wrote:
>> Of course, you can grab them from here:
>> https://drive.google.com/drive/folders/0B5duyI9SzNtWaXE0cjM1QzZJbVk?usp=shar
>> ing
>
> So the dumps of the two (using my own tool) are identical except for
> the expected difference due to the certificate. In particular neither
> image has any strange relocation types afaics, and both have the
> sort of unexpected, but also supposedly benign
> IMAGE_SCN_LNK_NRELOC_OVFL flag set for .bss. Hence I'm afraid ...
>
>> I've verified that xen-signed.efi boots with Secureboot enabled when
>> booted directly but doesn't boot through the shim.
>
> ... you'll need to do some debugging in order to figure out what's
> going on here. With the above the prime suspect is the shim though,
> fiddling with the image after loading it into memory. So perhaps
> dumping the .reloc section contents in order to compare it with
> what's in the image may be a suitable approach.
>
> Jan

Yeap, the shim pretty simply removed the .reloc section as it was
marked discardable and did the relocations for Xen. So with that
removed from the shim I no longer get the error and I see that the
dom0 kernel gets verified using the shim lock protocol. I still didn't
get dom0 to boot for some reason but that might be an unrelated issue
(and I have no serial console right now). Nevertheless, progress!

Tamas

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

Re: [Xen-devel] [Xen-users] UEFI Secure Boot Xen 4.9

[Tamas K Lengyel]

On Tue, Sep 5, 2017 at 12:26 PM, Tamas K Lengyel
<tamas.k.leng...@gmail.com> wrote:
> On Mon, Sep 4, 2017 at 6:40 AM, Daniel Kiper <daniel.ki...@oracle.com> wrote:
>> On Wed, Aug 30, 2017 at 10:16:23AM -0600, Tamas K Lengyel wrote:
>>> On Tue, Aug 29, 2017 at 2:01 PM, Daniel Kiper <daniel.ki...@oracle.com> 
>>> wrote:
>>> > Hey Tamas,
>>> >
>>> > Sorry for late reply. I was on vacation.
>>> >
>>> > On Tue, Aug 22, 2017 at 09:01:06PM -0600, Tamas K Lengyel wrote:
>>> >> On Tue, May 16, 2017 at 5:04 AM, Daniel Kiper <daniel.ki...@oracle.com> 
>>> >> wrote:
>>> >
>>> > [...]
>>> >
>>> >> > UEFI will verify shim secure boot signature then shim will verify GRUB2
>>> >> > signature then GRUB2 will verify (with shim protocol) Xen signature and
>>> >> > finally Xen will verify (with shim protocol) Linux kernel signature. 
>>> >> > Then
>>> >> > your kernel can verify modules using whatever you want.
>>> >> >
>>> >> >> I would be happy to work to help achieve this.
>>> >> >
>>> >> > There is a chance that I will have something very raw at the beginning
>>> >> > of June. If you wish to do tests drop me a line.
>>> >>
>>> >> Hi Daniel,
>>> >> is there any news on this? I would be interested in giving this a shot 
>>> >> too.
>>> >
>>> > Please look at
>>> >
>>> >   https://lists.xen.org/archives/html/xen-devel/2017-07/msg00982.html
>>> >
>>> > and at
>>> >
>>> >   https://lists.xen.org/archives/html/xen-devel/2017-07/msg00985.html
>>> >
>>> > Attachments contain the same patches as above but rebased on latest
>>> > GRUB2 and Xen git repositories.
>>> >
>>> > Due to some travel I am going to restart work on this in the second
>>> > half of September.
>>> >
>>> > If you have any questions please drop me a line.
>>> >
>>>
>>> Hi Daniel,
>>> thanks for the update, I'll give it a shot today to set it up. In a
>>> somewhat related note, are you aware of any work on getting secure
>>> boot + UEFI working in a guest? There is a PoC patch on OpenXT
>>> (https://github.com/OpenXT/xenclient-oe/pull/729) but was wondering if
>>> there are any parallel efforts ongoing.
>>
>> I do not follow this issue in detail. However, I suppose that if OVMF
>> supports UEFI secure boot (well, QEMU has to enable SMM support too;
>> I do not know does it work with Xen or not) then guest should work
>> without any issue. Just guessing...
>>
>
> Sure, was just wondering if you are aware of anyone looking at that.
>
> In other news I was able to get your patches working and have been
> able to boot with Secure boot enabled as far as shim -> signed grub ->
> signed linux without initrd. If I boot a signed version of Xen from
> grub it goes as far as setup_efi_pci but then the system reboots
> without anything else being printed on the screen. I haven't been able
> to debug it any further yet.
>

Daniel,
just FYI the xen.mb.efi generated with your patches causes pesign to segfault:

cms_pe_common.c:generate_digest:198 PE section ".text" has invalid address
Segmentation fault

Tamas

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

Re: [Xen-devel] Booting signed xen.efi through shim

[Tamas K Lengyel]

On Thu, Sep 14, 2017 at 12:06 PM, Jan Beulich  wrote:
 On 14.09.17 at 17:43,  wrote:
>> On Wed, Sep 13, 2017 at 11:42 AM, Jan Beulich  wrote:
>> On 13.09.17 at 16:40,  wrote:
 On Wed, Sep 13, 2017 at 3:21 AM, Jan Beulich  wrote:
 On 13.09.17 at 07:27,  wrote:
>>Sections:
>>Idx Name  Size  VMA   LMA   File off
>> Algn
>>  0 .text 0017a1ba  82d08020  82d08020  1000
>> 2**12
>>  CONTENTS, ALLOC, LOAD, CODE
>>  1 .rodata   000826a0  82d08040  82d08040  0017c000
>> 2**5
>>  CONTENTS, ALLOC, LOAD, DATA
>>  2 .buildid  0035  82d0804826a0  82d0804826a0  001fe6a0
>> 2**2
>>  CONTENTS, ALLOC, LOAD, READONLY, DATA
>>  3 .init 00077df0  82d08060  82d08060  001ff000
>> 2**12
>>  CONTENTS, ALLOC, LOAD, CODE, DATA
>>  4 .data.re  aa40  82d08080  82d08080  00277000
>> 2**7
>>  CONTENTS, ALLOC, LOAD, DATA
>>  5 .data 000105a8  82d08080b000  82d08080b000  00282000
>> 2**12
>>  CONTENTS, ALLOC, LOAD, DATA
>>  6 .bss  00143280  82d08082  82d08082  
>> 2**4
>>  ALLOC, RELOC
>
> Objdump is apparently ignoring a section attribute bit here - my
> own utility properly prints "bss" in addition to "read" (which presumably
> matches "ALLOC" above, albeit that's a bogus translation apparently
> applying ELF semantics to COFF). You'll want to check that bit 7 in the
> section attributes is set. I'm also puzzled by "RELOC", but I do see a
> matching bit dumped here; not sure why that's being set.

 Looking at it with readpe I get:

 Name:.bss
 Virtual Address: 0x82
 Physical Address:0x143280
 Size:0 (0 bytes)
 Pointer To Data: 0
 Relocations: 0
 Characteristics: 0xc180
  contains uninitialized data
  contains extended relocations
  is readable
  is writable

 So bit 7 is set AFAICT.
>>>
>>> Good.
>>>
> It is certainly the case that .bss style sections are expected to have a
> zero file offset, as there's no data for such sections inside the file 
> (note
> the missing "CONTENTS" above. So I would conclude that, unless the
> bss flag really got lost, it's a shim loader bug. Since other people can
> load xen.efi with the shim, that might be a problem with the particular
> version you're using.

 Perhaps, I'm using the latest master
 (e22a7b5b772dba6588dd955dc017e572f7e29784) from
 https://github.com/mjg59/shim, the one being linked to on the wiki. If
 there is a known good version, I would be happy to give that a shot
 and see if I can get it working.
>>>
>>> I have no idea. What I'd suggest you to try is to zap that stray
>>> "contains extended relocations" flag. I've written down to go hunt
>>> for where it comes from, but I don't have the time to do that right
>>> away.
>>
>> So I had made some progress using the shim from
>> https://github.com/rhboot/shim, it actually has been able to jump into
>> the signed xen.efi. However, Xen bails with the message "Unsupported
>> relocation type" which is in efi_arch_relocate_image.
>
> Iirc the dump you did send showed quite a few strange relocs;
> I wasn't sure whether these were just a result of the dumping
> utility not working well, but it now looks like the relocations
> really aren't right. Could you make available an un-signed
> xen.efi (which I understand works for you) and the corresponding
> signed one somewhere for analysis?
>

Of course, you can grab them from here:
https://drive.google.com/drive/folders/0B5duyI9SzNtWaXE0cjM1QzZJbVk?usp=sharing
I've verified that xen-signed.efi boots with Secureboot enabled when
booted directly but doesn't boot through the shim.

Tamas

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

Re: [Xen-devel] Booting signed xen.efi through shim

[Tamas K Lengyel]

On Wed, Sep 13, 2017 at 11:42 AM, Jan Beulich  wrote:
 On 13.09.17 at 16:40,  wrote:
>> On Wed, Sep 13, 2017 at 3:21 AM, Jan Beulich  wrote:
>> On 13.09.17 at 07:27,  wrote:
Sections:
Idx Name  Size  VMA   LMA   File off  
Algn
  0 .text 0017a1ba  82d08020  82d08020  1000  
 2**12
  CONTENTS, ALLOC, LOAD, CODE
  1 .rodata   000826a0  82d08040  82d08040  0017c000  
 2**5
  CONTENTS, ALLOC, LOAD, DATA
  2 .buildid  0035  82d0804826a0  82d0804826a0  001fe6a0  
 2**2
  CONTENTS, ALLOC, LOAD, READONLY, DATA
  3 .init 00077df0  82d08060  82d08060  001ff000  
 2**12
  CONTENTS, ALLOC, LOAD, CODE, DATA
  4 .data.re  aa40  82d08080  82d08080  00277000  
 2**7
  CONTENTS, ALLOC, LOAD, DATA
  5 .data 000105a8  82d08080b000  82d08080b000  00282000  
 2**12
  CONTENTS, ALLOC, LOAD, DATA
  6 .bss  00143280  82d08082  82d08082    
 2**4
  ALLOC, RELOC
>>>
>>> Objdump is apparently ignoring a section attribute bit here - my
>>> own utility properly prints "bss" in addition to "read" (which presumably
>>> matches "ALLOC" above, albeit that's a bogus translation apparently
>>> applying ELF semantics to COFF). You'll want to check that bit 7 in the
>>> section attributes is set. I'm also puzzled by "RELOC", but I do see a
>>> matching bit dumped here; not sure why that's being set.
>>
>> Looking at it with readpe I get:
>>
>> Name:.bss
>> Virtual Address: 0x82
>> Physical Address:0x143280
>> Size:0 (0 bytes)
>> Pointer To Data: 0
>> Relocations: 0
>> Characteristics: 0xc180
>>  contains uninitialized data
>>  contains extended relocations
>>  is readable
>>  is writable
>>
>> So bit 7 is set AFAICT.
>
> Good.
>
>>> It is certainly the case that .bss style sections are expected to have a
>>> zero file offset, as there's no data for such sections inside the file (note
>>> the missing "CONTENTS" above. So I would conclude that, unless the
>>> bss flag really got lost, it's a shim loader bug. Since other people can
>>> load xen.efi with the shim, that might be a problem with the particular
>>> version you're using.
>>
>> Perhaps, I'm using the latest master
>> (e22a7b5b772dba6588dd955dc017e572f7e29784) from
>> https://github.com/mjg59/shim, the one being linked to on the wiki. If
>> there is a known good version, I would be happy to give that a shot
>> and see if I can get it working.
>
> I have no idea. What I'd suggest you to try is to zap that stray
> "contains extended relocations" flag. I've written down to go hunt
> for where it comes from, but I don't have the time to do that right
> away.

So I had made some progress using the shim from
https://github.com/rhboot/shim, it actually has been able to jump into
the signed xen.efi. However, Xen bails with the message "Unsupported
relocation type" which is in efi_arch_relocate_image.

Tamas

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

Re: [Xen-devel] [PATCH 09/15] xen/x86: p2m: Use typesafe GFN in p2m_set_entry

[Tamas K Lengyel]

On Wed, Sep 13, 2017 at 11:59 AM, Julien Grall <julien.gr...@arm.com> wrote:
> Signed-off-by: Julien Grall <julien.gr...@arm.com>
>

I guess the rest of the mem_sharing codebase would benefit from moving
to the use gfn_t as well, clearing up some of the gfn conversion stuff
that's needed right now..

Acked-by: Tamas K Lengyel <ta...@tklengyel.com>

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

Re: [Xen-devel] Booting signed xen.efi through shim

[Tamas K Lengyel]

On Wed, Sep 13, 2017 at 3:21 AM, Jan Beulich  wrote:
 On 13.09.17 at 07:27,  wrote:
>>Sections:
>>Idx Name  Size  VMA   LMA   File off  Algn
>>  0 .text 0017a1ba  82d08020  82d08020  1000  
>> 2**12
>>  CONTENTS, ALLOC, LOAD, CODE
>>  1 .rodata   000826a0  82d08040  82d08040  0017c000  2**5
>>  CONTENTS, ALLOC, LOAD, DATA
>>  2 .buildid  0035  82d0804826a0  82d0804826a0  001fe6a0  2**2
>>  CONTENTS, ALLOC, LOAD, READONLY, DATA
>>  3 .init 00077df0  82d08060  82d08060  001ff000  
>> 2**12
>>  CONTENTS, ALLOC, LOAD, CODE, DATA
>>  4 .data.re  aa40  82d08080  82d08080  00277000  2**7
>>  CONTENTS, ALLOC, LOAD, DATA
>>  5 .data 000105a8  82d08080b000  82d08080b000  00282000  
>> 2**12
>>  CONTENTS, ALLOC, LOAD, DATA
>>  6 .bss  00143280  82d08082  82d08082    2**4
>>  ALLOC, RELOC
>
> Objdump is apparently ignoring a section attribute bit here - my
> own utility properly prints "bss" in addition to "read" (which presumably
> matches "ALLOC" above, albeit that's a bogus translation apparently
> applying ELF semantics to COFF). You'll want to check that bit 7 in the
> section attributes is set. I'm also puzzled by "RELOC", but I do see a
> matching bit dumped here; not sure why that's being set.

Looking at it with readpe I get:

Name:.bss
Virtual Address: 0x82
Physical Address:0x143280
Size:0 (0 bytes)
Pointer To Data: 0
Relocations: 0
Characteristics: 0xc180
 contains uninitialized data
 contains extended relocations
 is readable
 is writable

So bit 7 is set AFAICT.

>
> It is certainly the case that .bss style sections are expected to have a
> zero file offset, as there's no data for such sections inside the file (note
> the missing "CONTENTS" above. So I would conclude that, unless the
> bss flag really got lost, it's a shim loader bug. Since other people can
> load xen.efi with the shim, that might be a problem with the particular
> version you're using.

Perhaps, I'm using the latest master
(e22a7b5b772dba6588dd955dc017e572f7e29784) from
https://github.com/mjg59/shim, the one being linked to on the wiki. If
there is a known good version, I would be happy to give that a shot
and see if I can get it working.

Thanks,
Tamas

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

[Xen-devel] Booting signed xen.efi through shim

[Tamas K Lengyel]

Hi all,
for the last couple weeks I've been poking around the options
available to get Xen booted on a Secureboot enabled box. My goal is to
extend the chain of trust to the dom0 kernel. According to
https://wiki.xenproject.org/wiki/Xen_EFI this is something that's
supposed to be supported out-of-the-box right now via the shim
protocol. However, when I try to boot a signed xen.efi (4.10 unstable
head) through shim I get the error "Section 6 is inside image header"
and shim refuses to load Xen. OTOH I had been able to boot a
custom-compiled grub2 from the shim no problem with Secureboot
enabled. The signed xen.efi also boots fine with Secureboot enabled if
booted directly as an EFI application (but then no dom0 verification
is done AFAIU). Does anyone have any pointers on what's going on with
booting through the shim?

Thanks,
Tamas

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

Re: [Xen-devel] [PATCH v10 0/3] Notify monitor when emulating an unimplemented instruction

[Tamas K Lengyel]

On Wed, Sep 6, 2017 at 7:48 AM, Petre Pircalabu
 wrote:
> This patchset implements a mechanism which allows XEN to send first an event
> if the emulator encountered an unsupported instruction.
> The monitor application can choose to mitigate the error, for example to 
> singlestep
> the instruction using the real processor and then resume execution of the 
> normal
> instruction flow.
>
> This feature was tested using a modified version of XTF:
> https://github.com/petrepircalabu/xen-test-framework/tree/emul_unimpl

Hi Petre,
thanks for sharing that! Do you have any plans to upstream that to XTF
so we can have some automated tests for other parts of the monitor
code as well?

Tamas

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

Re: [Xen-devel] [Xen-users] UEFI Secure Boot Xen 4.9

[Tamas K Lengyel]

On Mon, Sep 4, 2017 at 6:40 AM, Daniel Kiper <daniel.ki...@oracle.com> wrote:
> On Wed, Aug 30, 2017 at 10:16:23AM -0600, Tamas K Lengyel wrote:
>> On Tue, Aug 29, 2017 at 2:01 PM, Daniel Kiper <daniel.ki...@oracle.com> 
>> wrote:
>> > Hey Tamas,
>> >
>> > Sorry for late reply. I was on vacation.
>> >
>> > On Tue, Aug 22, 2017 at 09:01:06PM -0600, Tamas K Lengyel wrote:
>> >> On Tue, May 16, 2017 at 5:04 AM, Daniel Kiper <daniel.ki...@oracle.com> 
>> >> wrote:
>> >
>> > [...]
>> >
>> >> > UEFI will verify shim secure boot signature then shim will verify GRUB2
>> >> > signature then GRUB2 will verify (with shim protocol) Xen signature and
>> >> > finally Xen will verify (with shim protocol) Linux kernel signature. 
>> >> > Then
>> >> > your kernel can verify modules using whatever you want.
>> >> >
>> >> >> I would be happy to work to help achieve this.
>> >> >
>> >> > There is a chance that I will have something very raw at the beginning
>> >> > of June. If you wish to do tests drop me a line.
>> >>
>> >> Hi Daniel,
>> >> is there any news on this? I would be interested in giving this a shot 
>> >> too.
>> >
>> > Please look at
>> >
>> >   https://lists.xen.org/archives/html/xen-devel/2017-07/msg00982.html
>> >
>> > and at
>> >
>> >   https://lists.xen.org/archives/html/xen-devel/2017-07/msg00985.html
>> >
>> > Attachments contain the same patches as above but rebased on latest
>> > GRUB2 and Xen git repositories.
>> >
>> > Due to some travel I am going to restart work on this in the second
>> > half of September.
>> >
>> > If you have any questions please drop me a line.
>> >
>>
>> Hi Daniel,
>> thanks for the update, I'll give it a shot today to set it up. In a
>> somewhat related note, are you aware of any work on getting secure
>> boot + UEFI working in a guest? There is a PoC patch on OpenXT
>> (https://github.com/OpenXT/xenclient-oe/pull/729) but was wondering if
>> there are any parallel efforts ongoing.
>
> I do not follow this issue in detail. However, I suppose that if OVMF
> supports UEFI secure boot (well, QEMU has to enable SMM support too;
> I do not know does it work with Xen or not) then guest should work
> without any issue. Just guessing...
>

Sure, was just wondering if you are aware of anyone looking at that.

In other news I was able to get your patches working and have been
able to boot with Secure boot enabled as far as shim -> signed grub ->
signed linux without initrd. If I boot a signed version of Xen from
grub it goes as far as setup_efi_pci but then the system reboots
without anything else being printed on the screen. I haven't been able
to debug it any further yet.

Tamas

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

Re: [Xen-devel] [PATCH v6] common/vm_event: Initialize vm_event lists on domain creation

[Tamas K Lengyel]

On Wed, Aug 30, 2017 at 3:04 AM, Alexandru Isaila
<aisa...@bitdefender.com> wrote:
> The patch splits the vm_event into three structures:vm_event_share,
> vm_event_paging, vm_event_monitor. The allocation for the
> structure is moved to vm_event_enable so that it can be
> allocated/init when needed and freed in vm_event_disable.
>
> Signed-off-by: Alexandru Isaila <aisa...@bitdefender.com>

Acked-by: Tamas K Lengyel <ta...@tklengyel.com>

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

Re: [Xen-devel] [RFC PATCH 1/4] arm/monitor: Introduce monitoring of single-step events

[Tamas K Lengyel]

> diff --git a/xen/include/asm-arm/monitor.h b/xen/include/asm-arm/monitor.h
> index 7567be66bd..66c7fe14fe 100644
> --- a/xen/include/asm-arm/monitor.h
> +++ b/xen/include/asm-arm/monitor.h
> @@ -57,12 +57,15 @@ static inline uint32_t 
> arch_monitor_get_capabilities(struct domain *d)
>  {
>  uint32_t capabilities = 0;
>
> -capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST |
> +capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP |
> +1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST |
>  1U << XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL);

Please append the new capability bit instead of prepending.

Tamas

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

Re: [Xen-devel] [Xen-users] UEFI Secure Boot Xen 4.9

[Tamas K Lengyel]

On Tue, Aug 29, 2017 at 2:01 PM, Daniel Kiper <daniel.ki...@oracle.com> wrote:
> Hey Tamas,
>
> Sorry for late reply. I was on vacation.
>
> On Tue, Aug 22, 2017 at 09:01:06PM -0600, Tamas K Lengyel wrote:
>> On Tue, May 16, 2017 at 5:04 AM, Daniel Kiper <daniel.ki...@oracle.com> 
>> wrote:
>
> [...]
>
>> > UEFI will verify shim secure boot signature then shim will verify GRUB2
>> > signature then GRUB2 will verify (with shim protocol) Xen signature and
>> > finally Xen will verify (with shim protocol) Linux kernel signature. Then
>> > your kernel can verify modules using whatever you want.
>> >
>> >> I would be happy to work to help achieve this.
>> >
>> > There is a chance that I will have something very raw at the beginning
>> > of June. If you wish to do tests drop me a line.
>>
>> Hi Daniel,
>> is there any news on this? I would be interested in giving this a shot too.
>
> Please look at
>
>   https://lists.xen.org/archives/html/xen-devel/2017-07/msg00982.html
>
> and at
>
>   https://lists.xen.org/archives/html/xen-devel/2017-07/msg00985.html
>
> Attachments contain the same patches as above but rebased on latest
> GRUB2 and Xen git repositories.
>
> Due to some travel I am going to restart work on this in the second
> half of September.
>
> If you have any questions please drop me a line.
>

Hi Daniel,
thanks for the update, I'll give it a shot today to set it up. In a
somewhat related note, are you aware of any work on getting secure
boot + UEFI working in a guest? There is a PoC patch on OpenXT
(https://github.com/OpenXT/xenclient-oe/pull/729) but was wondering if
there are any parallel efforts ongoing.

Thanks,
Tamas

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

Re: [Xen-devel] [PATCH v5] common/vm_event: Initialize vm_event lists on domain creation

[Tamas K Lengyel]

On Tue, Aug 29, 2017 at 9:59 AM, Wei Liu  wrote:
> On Tue, Aug 29, 2017 at 05:17:05PM +0300, Alexandru Isaila wrote:
> [...]
>>
>>  /**
>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>> index b22aacc..30f507b 100644
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -363,9 +363,6 @@ struct domain *domain_create(domid_t domid, unsigned int 
>> domcr_flags,
>>  poolid = 0;
>>
>>  err = -ENOMEM;
>> -d->vm_event = xzalloc(struct vm_event_per_domain);
>> -if ( !d->vm_event )
>> -goto fail;
>>
>>  d->pbuf = xzalloc_array(char, DOMAIN_PBUF_SIZE);
>>  if ( !d->pbuf )
>> @@ -403,7 +400,6 @@ struct domain *domain_create(domid_t domid, unsigned int 
>> domcr_flags,
>>  if ( hardware_domain == d )
>>  hardware_domain = old_hwdom;
>>  atomic_set(>refcnt, DOMAIN_DESTROYED);
>> -xfree(d->vm_event);
>>  xfree(d->pbuf);
>>  if ( init_status & INIT_arch )
>>  arch_domain_destroy(d);
>> @@ -820,7 +816,14 @@ static void complete_domain_destroy(struct rcu_head 
>> *head)
>>  free_xenoprof_pages(d);
>>  #endif
>>
>> -xfree(d->vm_event);
>> +#ifdef CONFIG_HAS_MEM_PAGING
>> +xfree(d->vm_event_paging);
>> +#endif
>> +xfree(d->vm_event_monitor);
>
> Why do you unconditionally xfree these vm_event_monitor while you don't
> unconditionally allocate them?
>
> Not that this is strictly a bug but this deviates from the original
> behaviour.

Isn't xfree NULL safe?

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

Re: [Xen-devel] [PATCH v5] common/vm_event: Initialize vm_event lists on domain creation

[Tamas K Lengyel]

On Tue, Aug 29, 2017 at 8:17 AM, Alexandru Isaila
 wrote:
> The patch splits the vm_event into three structures:vm_event_share,
> vm_event_paging, vm_event_monitor. The allocation for the
> structure is moved to vm_event_enable so that it can be
> allocated/init when needed and freed in vm_event_disable.
>
> Signed-off-by: Alexandru Isaila 
>
> ---
> Changes since V4:
> - Replaced all NULL checks with vm_event_check_ring
>
> Note: Did not test on arm, compliled on arm and x86.

This looks good to me as is but could you at least check with the
xen-access test tool on x86 that it still works as expected?

Thanks,
Tamas

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

Re: [Xen-devel] [PATCH v10] x86/hvm: Allow guest_request vm_events coming from userspace

[Tamas K Lengyel]

On Tue, Aug 29, 2017 at 3:23 AM, Alexandru Isaila
<aisa...@bitdefender.com> wrote:
> In some introspection usecases, an in-guest agent needs to communicate
> with the external introspection agent.  An existing mechanism is
> HVMOP_guest_request_vm_event, but this is restricted to kernel usecases
> like all other hypercalls.
>
> Introduce a mechanism whereby the introspection agent can whitelist the
> use of HVMOP_guest_request_vm_event directly from userspace.
>
> Signed-off-by: Alexandru Isaila <aisa...@bitdefender.com>
> Acked-by: Wei Liu <wei.l...@citrix.com>

Acked-by: Tamas K Lengyel <ta...@tklengyel.com>

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

Re: [Xen-devel] [PATCH v10] x86/hvm: Allow guest_request vm_events coming from userspace

[Tamas K Lengyel]

On Tue, Aug 29, 2017 at 3:36 AM, Jan Beulich  wrote:
 On 29.08.17 at 11:23,  wrote:
>> In some introspection usecases, an in-guest agent needs to communicate
>> with the external introspection agent.  An existing mechanism is
>> HVMOP_guest_request_vm_event, but this is restricted to kernel usecases
>> like all other hypercalls.
>>
>> Introduce a mechanism whereby the introspection agent can whitelist the
>> use of HVMOP_guest_request_vm_event directly from userspace.
>>
>> Signed-off-by: Alexandru Isaila 
>> Acked-by: Wei Liu 
>
> For the pieces it applies to
> Acked-by: Jan Beulich 
>
> However, as I keep looking at pieces which shouldn't really require
> my attention, I've noticed one more cosmetic issue:
>
>> --- a/xen/include/asm-arm/monitor.h
>> +++ b/xen/include/asm-arm/monitor.h
>> @@ -26,6 +26,12 @@
>>  #include 
>>
>>  static inline
>> +void arch_monitor_allow_userspace(struct domain *d, bool allow_userspace)
>> +{
>> +return;
>> +}
>
> I don't see the point of the return statement here. But I'll leave
> it to the ARM maintainers, and it would be easy to drop while
> committing if no other issues are going to arise.
>

I would bet that a sane compiler wouldn't generate anything for that
with or without return in there but sure, if we can drop it then why
not.

Tamas

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

Re: [Xen-devel] [PATCH v8] x86/hvm: Allow guest_request vm_events coming from userspace

[Tamas K Lengyel]

On Mon, Aug 28, 2017 at 7:29 AM, Jan Beulich  wrote:
 On 28.08.17 at 14:51,  wrote:
>> --- a/xen/include/asm-arm/monitor.h
>> +++ b/xen/include/asm-arm/monitor.h
>> @@ -26,6 +26,12 @@
>>  #include 
>>
>>  static inline
>> +void arch_allow_userspace(struct domain *d, uint8_t allow_userspace)
>> +{
>> +return;
>> +}
>
> I'm sorry for noticing this only now, but the name of the hook really
> ought to have vm_event or monitor in it - it's way too generic the
> way it is now.
>

+1 I've just stated that too on apparently the previous version of the patch =)

Tamas

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

Re: [Xen-devel] [PATCH v7] x86/hvm: Allow guest_request vm_events coming from userspace

[Tamas K Lengyel]

On Mon, Aug 28, 2017 at 5:10 AM, Jan Beulich  wrote:
 On 28.08.17 at 11:38,  wrote:
>> In some introspection usecases, an in-guest agent needs to communicate
>> with the external introspection agent.  An existing mechanism is
>> HVMOP_guest_request_vm_event, but this is restricted to kernel usecases
>> like all other hypercalls.
>>
>> Introduce a mechanism whereby the introspection agent can whitelist the
>> use of HVMOP_guest_request_vm_event directly from userspace.
>>
>> Signed-off-by: Alexandru Isaila 
>
> For the parts it is applicable to:
> Acked-by: Jan Beulich 
>
> I'd like to note though that I find it a little odd for >arch to be
> passed to a hook, instead of just d. But it'll be the maintainers of
> that code to approve (or not) of that.
>

Indeed, I don't see d->arch being passed like this anywhere else
either. I don't think it breaks anything but for stylistic reasons it
might be better to conform here too.

Tamas

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

Re: [Xen-devel] [PATCH v7] x86/hvm: Allow guest_request vm_events coming from userspace

[Tamas K Lengyel]

> diff --git a/xen/common/monitor.c b/xen/common/monitor.c
> index 451f42f..0c3e645 100644
> --- a/xen/common/monitor.c
> +++ b/xen/common/monitor.c
> @@ -75,6 +75,7 @@ int monitor_domctl(struct domain *d, struct 
> xen_domctl_monitor_op *mop)
>  domain_pause(d);
>  d->monitor.guest_request_sync = mop->u.guest_request.sync;
>  d->monitor.guest_request_enabled = requested_status;
> +arch_allow_userspace(>arch, mop->u.guest_request.allow_userspace);

Please use the appropriate prefix with this function, ie.
arch_monitor_allow_userspace.

Thanks,
Tamas

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

Re: [Xen-devel] [PATCH v4] common/vm_event: Initialize vm_event lists on domain creation

[Tamas K Lengyel]

On Mon, Aug 28, 2017 at 4:54 AM, Alexandru Isaila
 wrote:
> The patch splits the vm_event into three structures:vm_event_share,
> vm_event_paging, vm_event_monitor. The allocation for the
> structure is moved to vm_event_enable so that it can be
> allocated/init when needed and freed in vm_event_disable.
>
> Signed-off-by: Alexandru Isaila 
>
> ---
> Changes since V3:
> - Moved the d->vm_event_paging to the if below in the
> assign_device function
>
> Note: Did not test on arm, compliled on arm and x86.
> ---
>  xen/arch/arm/mem_access.c |   2 +-
>  xen/arch/x86/mm/mem_access.c  |   2 +-
>  xen/arch/x86/mm/mem_paging.c  |   2 +-
>  xen/arch/x86/mm/mem_sharing.c |   4 +-
>  xen/arch/x86/mm/p2m.c |  10 +--
>  xen/common/domain.c   |  13 ++--
>  xen/common/mem_access.c   |   2 +-
>  xen/common/monitor.c  |   4 +-
>  xen/common/vm_event.c | 156 
> +-
>  xen/drivers/passthrough/pci.c |   2 +-
>  xen/include/xen/sched.h   |  18 ++---
>  11 files changed, 124 insertions(+), 91 deletions(-)
>
> diff --git a/xen/arch/arm/mem_access.c b/xen/arch/arm/mem_access.c
> index e0888bb..a7f0cae 100644
> --- a/xen/arch/arm/mem_access.c
> +++ b/xen/arch/arm/mem_access.c
> @@ -256,7 +256,7 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, 
> const struct npfec npfec)
>  }
>
>  /* Otherwise, check if there is a vm_event monitor subscriber */
> -if ( !vm_event_check_ring(>domain->vm_event->monitor) )
> +if ( !vm_event_check_ring(v->domain->vm_event_monitor) )
>  {
>  /* No listener */
>  if ( p2m->access_required )
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index 5adaf6d..414e38f 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -179,7 +179,7 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long 
> gla,
>  gfn_unlock(p2m, gfn, 0);
>
>  /* Otherwise, check if there is a memory event listener, and send the 
> message along */
> -if ( !vm_event_check_ring(>vm_event->monitor) || !req_ptr )
> +if ( !vm_event_check_ring(d->vm_event_monitor) || !req_ptr )
>  {
>  /* No listener */
>  if ( p2m->access_required )
> diff --git a/xen/arch/x86/mm/mem_paging.c b/xen/arch/x86/mm/mem_paging.c
> index a049e0d..20214ac 100644
> --- a/xen/arch/x86/mm/mem_paging.c
> +++ b/xen/arch/x86/mm/mem_paging.c
> @@ -43,7 +43,7 @@ int 
> mem_paging_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_paging_op_t) arg)
>  goto out;
>
>  rc = -ENODEV;
> -if ( unlikely(!d->vm_event->paging.ring_page) )
> +if ( !d->vm_event_paging || unlikely(!d->vm_event_paging->ring_page) )

So you are adding an extra NULL check here for d->vm_event_paging.
Perhaps now we should have that NULL check used in all cases by adding
it to vm_event_check_ring and we should use that across all cases..

> diff --git a/xen/common/mem_access.c b/xen/common/mem_access.c
> index 19f63bb..10ea4ae 100644
> --- a/xen/common/mem_access.c
> +++ b/xen/common/mem_access.c
> @@ -52,7 +52,7 @@ int mem_access_memop(unsigned long cmd,
>  goto out;
>
>  rc = -ENODEV;
> -if ( unlikely(!d->vm_event->monitor.ring_page) )
> +if ( !d->vm_event_monitor || unlikely(!d->vm_event_monitor->ring_page) )

... like here ^ ...

> @@ -187,41 +194,44 @@ void vm_event_wake(struct domain *d, struct 
> vm_event_domain *ved)
>  vm_event_wake_blocked(d, ved);
>  }
>
> -static int vm_event_disable(struct domain *d, struct vm_event_domain *ved)
> +static int vm_event_disable(struct domain *d, struct vm_event_domain **ved)
>  {
> -if ( ved->ring_page )
> +if ( *ved && (*ved)->ring_page )

... and here ^ ...


> @@ -267,6 +277,9 @@ void vm_event_put_request(struct domain *d,
>  RING_IDX req_prod;
>  struct vcpu *curr = current;
>
> +if( !ved )

... and here ^ ...

> +return;
> +
>  if ( curr->domain != d )
>  {
>  req->flags |= VM_EVENT_FLAG_FOREIGN;
> @@ -434,6 +447,9 @@ void vm_event_resume(struct domain *d, struct 
> vm_event_domain *ved)
>
>  void vm_event_cancel_slot(struct domain *d, struct vm_event_domain *ved)
>  {
> +if( !ved )

... and here ^ ...

> +return;
> +
>  vm_event_ring_lock(ved);
>  vm_event_release_slot(d, ved);
>  vm_event_ring_unlock(ved);
> @@ -500,6 +516,9 @@ bool_t vm_event_check_ring(struct vm_event_domain *ved)
>  int __vm_event_claim_slot(struct domain *d, struct vm_event_domain *ved,
>bool_t allow_sleep)
>  {
> +if ( !ved )

... and here ^ ...

> +return -EOPNOTSUPP;
> +
>  if ( (current->domain == d) && allow_sleep )
>  return vm_event_wait_slot(ved);
>  else
> @@ -510,24 +529,30 @@ int __vm_event_claim_slot(struct domain *d, struct 
> vm_event_domain *ved,
>  /* Registered with Xen-bound event channel for incoming notifications. */
>  static 

Re: [Xen-devel] [PATCH v6] x86/hvm: Allow guest_request vm_events coming from userspace

[Tamas K Lengyel]

On Fri, Aug 25, 2017 at 7:44 AM, Jan Beulich  wrote:
 On 25.08.17 at 15:00,  wrote:
>> On Vi, 2017-08-25 at 06:13 -0600, Jan Beulich wrote:
>>> >
>>> > >
>>> > > >
>>> > > > On 17.08.17 at 13:50,  wrote:
>>> > --- a/xen/common/monitor.c
>>> > +++ b/xen/common/monitor.c
>>> > @@ -75,6 +75,7 @@ int monitor_domctl(struct domain *d, struct
>>> > xen_domctl_monitor_op *mop)
>>> >  domain_pause(d);
>>> >  d->monitor.guest_request_sync = mop->u.guest_request.sync;
>>> >  d->monitor.guest_request_enabled = requested_status;
>>> > +d->arch.monitor.guest_request_userspace_enabled = mop-
>>> > >u.guest_request.allow_userspace;
>>> This breaks the build on ARM.
>> There are 2 solutions, I can move the case in x86/monitor.c in
>> the arch_monitor_domctl_event function or I can make a arch specific
>> function that does the assignment in the x86 case and does nothing in
>> the arm case. What approach do you prefer?
>
> That's a question to the maintainers of that code. What I care
> about is that patches touching common code please are at least
> build-checked on the other architecture before submission.
>

Ough, yes, please build-check your code on both architectures before
sending them. As for which route to take I prefer in this case doing
the arch specific function that does the assignment when needed and is
empty when not. The function itself could probably be declared as
static inline too.

Tamas

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

Re: [Xen-devel] [PATCH v2] common/vm_event: Initialize vm_event lists on domain creation

[Tamas K Lengyel]

On Thu, Aug 24, 2017 at 9:24 AM, Jan Beulich  wrote:
 On 24.08.17 at 17:17,  wrote:
>> On Jo, 2017-08-24 at 07:24 -0600, Jan Beulich wrote:
>>> > @@ -500,6 +519,9 @@ bool_t vm_event_check_ring(struct
>>> > vm_event_domain *ved)
>>> >  int __vm_event_claim_slot(struct domain *d, struct vm_event_domain
>>> > *ved,
>>> >bool_t allow_sleep)
>>> >  {
>>> > +if ( !ved )
>>> > +return -ENOSYS;
>>> I don't think ENOSYS is correct here.
>> Can you tell me what is the preferred return value here?
>
> -EOPNOTSUPP is what we commonly use in such cases.
>
>>> > @@ -599,7 +621,6 @@ int vm_event_domctl(struct domain *d,
>>> > xen_domctl_vm_event_op_t *vec,
>>> >  #ifdef CONFIG_HAS_MEM_PAGING
>>> >  case XEN_DOMCTL_VM_EVENT_OP_PAGING:
>>> >  {
>>> > -struct vm_event_domain *ved = >vm_event->paging;
>>> Dropping this local variable (and similar ones below) pointlessly
>>> increases the size of this patch.
>> I have dropped the local var because in case of a XEN_VM_EVENT_ENABLE
>> d->vm_event_... is allocated in the vm_event_enable function below so
>> it will allocate mem for the local variable.
>
> I don't see how that renders the local variable useless. But anyway,
> its the maintainers of that code to judge.

I guess the reason for dropping it is that vm_event_enable will place
the location of the structure into the pointer that was passed in, so
if you are passing in a pointer that was locally declared you would
then still have to copy it back to d->vm_event_ which doesn't make
much sense. IMHO it's fine how it's changed in the patch.

Tamas

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

Re: [Xen-devel] [PATCH v2] common/vm_event: Initialize vm_event lists on domain creation

[Tamas K Lengyel]

On Thu, Aug 24, 2017 at 5:48 AM, Alexandru Isaila
 wrote:
> The patch splits the vm_event into three structures:vm_event_share,
> vm_event_paging, vm_event_monitor. The allocation for the
> structure is moved to vm_event_enable so that it can be
> allocated/init when needed and freed in vm_event_disable.
>
> Signed-off-by: Alexandru Isaila 

Thanks for doing this patch, I think it improves the code a lot!


> @@ -51,8 +51,7 @@ int mem_access_memop(unsigned long cmd,
>  if ( rc )
>  goto out;
>

Why are you removing setting the rc below?

> -rc = -ENODEV;
> -if ( unlikely(!d->vm_event->monitor.ring_page) )
> +if ( !d->vm_event_monitor || unlikely(!d->vm_event_monitor->ring_page) )
>  goto out;
>
>  switch ( mao.op )

...

> @@ -187,39 +194,45 @@ void vm_event_wake(struct domain *d, struct 
> vm_event_domain *ved)
>  vm_event_wake_blocked(d, ved);
>  }
>
> -static int vm_event_disable(struct domain *d, struct vm_event_domain *ved)
> +static int vm_event_disable(struct domain *d, struct vm_event_domain **ved)
>  {

I think you should check for *ved and *ved->ring_page all in one go below.

> -if ( ved->ring_page )
> +if ( !*ved )
> +return 0;
> +
> +if ( (*ved)->ring_page )
>  {
>  struct vcpu *v;
>

Thanks,
Tamas

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

Re: [Xen-devel] [Xen-users] UEFI Secure Boot Xen 4.9

[Tamas K Lengyel]

On Tue, May 16, 2017 at 5:04 AM, Daniel Kiper  wrote:
> On Mon, May 15, 2017 at 07:09:54PM +, Bill Jacobs (billjac) wrote:
>> > -Original Message-
>> > From: Daniel Kiper [mailto:daniel.ki...@oracle.com]
>> > Sent: Monday, May 15, 2017 6:13 AM
>> > To: Bill Jacobs (billjac) ; george.dun...@citrix.com
>> > Cc: xen-devel@lists.xen.org; xen-us...@lists.xen.org
>> > Subject: Re: [Xen-users] UEFI Secure Boot Xen 4.9
>> >
>> > Hey,
>> >
>> > CC-ing Xen-devel to spread some knowledge about the issue.
>> >
>> > On Mon, May 15, 2017 at 10:42:23AM +0100, George Dunlap wrote:
>> > > On Wed, May 10, 2017 at 11:36 PM, Bill Jacobs (billjac)
>> > >  wrote:
>> > > > Hi all
>> > > >
>> > > > I gather that with 4.9, UEFI secure boot of Xen should be possible.
>> > > >
>> > > > Is this true?
>> > > >
>> > > > If so, what are the options for utilizing UEFI secure boot? Do I
>> > > > need a MSFT-signed shim or grub? Any special changes required for
>> > > > Xen kernel
>> > > > (signing?) or has that been done?
>> > >
>> > > Bill,
>> > >
>> > > I guess in part it depends on what you mean by "utilizing UEFI secure
>> > > boot".  If you simply want to boot an unsigned Xen on a UEFI system
>> > > with SecureBoot enabled, then grub would probably work.  If you want
>> > > to actually do the full SecureBoot thing -- where you have grub check
>> > > Xen's signature and that of the kernel and initrd, you probably need a
>> > > bit more.
>> > >
>> > > Daniel,
>> > >
>> > > Is there any good documentation on this?  The Xen EFI guide
>> > > (https://wiki.xenproject.org/wiki/Xen_EFI) mentions the shim, but
>> > > doesn't go into detail about how to sign a binary 
>> >
>> > Unfortunately I do not know anything like that. As you said in general 
>> > shim is
>> > supported. Sadly, it works only if you load xen.efi directly from EFI.
>> > __Upstream__ GRUB2 has not have support for shim yet. I am working on it
>> > (shim support via GRUB2 requires also some changes in Xen). I hope that I 
>> > will
>> > have something which works before Xen conf in Budapest.
>> >
>> > If you wish to use shim with xen.efi then you have to sign xen.efi and 
>> > vmlinux
>> > with your key using sbsign or pesign. The process works in the same way 
>> > like in
>> > case vmlinux alone. Of course you have to install your public key into MOK
>> > before enabling secure boot.
>> >
>> > Daniel
>>
>> Yes, there are options in how this is achievable, and the solutions may be 
>> different.
>>
>> We are targeting a secure boot chain from UEFI fw to .ko, using same signing.
>> In our case would skip shim and reduce attack surface, but it appears that 
>> the mechanisms
>> 'out there' for passing pub key (cert) from UEFI db to Linux chainring 
>> require shim to do
>> the work. Is that accurate? Does it have to be the case? I don't see why.
>
> AIUI, if EFI secure boot is enabled then EFI verifies signatures of every
> loaded/executed PE file. Unfortunately, you are not able to use secure boot
> protocol directly to verify yourself PE's loaded from your app. So, this is
> one of reasons why shim was introduced. It exposes protocol which can be
> used by you to do verification.
>
>> For us, ideal case is :
>> UEFI fw -> (signed)GRUB2.efi->Multiboot2->Xen(signed .ko)
>
> AFAICT, it is not possible. We should do following thing:
>
>   UEFI -> shim -> GRUB2 -> Multiboot2 -> Xen/Linux/etc.
>
> UEFI will verify shim secure boot signature then shim will verify GRUB2
> signature then GRUB2 will verify (with shim protocol) Xen signature and
> finally Xen will verify (with shim protocol) Linux kernel signature. Then
> your kernel can verify modules using whatever you want.
>
>> I would be happy to work to help achieve this.
>
> There is a chance that I will have something very raw at the beginning
> of June. If you wish to do tests drop me a line.

Hi Daniel,
is there any news on this? I would be interested in giving this a shot too.

Thanks,
Tamas

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

Re: [Xen-devel] [PATCH v6] x86/hvm: Allow guest_request vm_events coming from userspace

[Tamas K Lengyel]

On Thu, Aug 17, 2017 at 5:50 AM, Alexandru Isaila
<aisa...@bitdefender.com> wrote:
> In some introspection usecases, an in-guest agent needs to communicate
> with the external introspection agent.  An existing mechanism is
> HVMOP_guest_request_vm_event, but this is restricted to kernel usecases
> like all other hypercalls.
>
> Introduce a mechanism whereby the introspection agent can whitelist the
> use of HVMOP_guest_request_vm_event directly from userspace.
>
> Signed-off-by: Alexandru Isaila <aisa...@bitdefender.com>

Acked-by: Tamas K Lengyel <ta...@tklengyel.com>

>
> ---
> Changes since V5:
> - Added the bool allow_userspace to the xc_monitor_guest_request
>  function
> ---
>  tools/libxc/include/xenctrl.h |  2 +-
>  tools/libxc/xc_monitor.c  |  3 ++-
>  xen/arch/x86/hvm/hypercall.c  |  5 +
>  xen/common/monitor.c  |  1 +
>  xen/include/asm-x86/domain.h  | 19 ++-
>  xen/include/public/domctl.h   |  1 +
>  6 files changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index bde8313..a3d0929 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -2021,7 +2021,7 @@ int xc_monitor_software_breakpoint(xc_interface *xch, 
> domid_t domain_id,
>  int xc_monitor_descriptor_access(xc_interface *xch, domid_t domain_id,
>   bool enable);
>  int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id,
> - bool enable, bool sync);
> + bool enable, bool sync, bool allow_userspace);
>  int xc_monitor_debug_exceptions(xc_interface *xch, domid_t domain_id,
>  bool enable, bool sync);
>  int xc_monitor_cpuid(xc_interface *xch, domid_t domain_id, bool enable);
> diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
> index b44ce93..a677820 100644
> --- a/tools/libxc/xc_monitor.c
> +++ b/tools/libxc/xc_monitor.c
> @@ -147,7 +147,7 @@ int xc_monitor_descriptor_access(xc_interface *xch, 
> domid_t domain_id,
>  }
>
>  int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id, bool 
> enable,
> - bool sync)
> + bool sync, bool allow_userspace)
>  {
>  DECLARE_DOMCTL;
>
> @@ -157,6 +157,7 @@ int xc_monitor_guest_request(xc_interface *xch, domid_t 
> domain_id, bool enable,
>  : XEN_DOMCTL_MONITOR_OP_DISABLE;
>  domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST;
>  domctl.u.monitor_op.u.guest_request.sync = sync;
> +domctl.u.monitor_op.u.guest_request.allow_userspace = enable ? 
> allow_userspace : false;
>
>  return do_domctl(xch, );
>  }
> diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
> index e7238ce..5742dd1 100644
> --- a/xen/arch/x86/hvm/hypercall.c
> +++ b/xen/arch/x86/hvm/hypercall.c
> @@ -155,6 +155,11 @@ int hvm_hypercall(struct cpu_user_regs *regs)
>  /* Fallthrough to permission check. */
>  case 4:
>  case 2:
> +if ( currd->arch.monitor.guest_request_userspace_enabled &&
> +eax == __HYPERVISOR_hvm_op &&
> +(mode == 8 ? regs->rdi : regs->ebx) == 
> HVMOP_guest_request_vm_event )
> +break;
> +
>  if ( unlikely(hvm_get_cpl(curr)) )
>  {
>  default:
> diff --git a/xen/common/monitor.c b/xen/common/monitor.c
> index 451f42f..20463e0 100644
> --- a/xen/common/monitor.c
> +++ b/xen/common/monitor.c
> @@ -75,6 +75,7 @@ int monitor_domctl(struct domain *d, struct 
> xen_domctl_monitor_op *mop)
>  domain_pause(d);
>  d->monitor.guest_request_sync = mop->u.guest_request.sync;
>  d->monitor.guest_request_enabled = requested_status;
> +d->arch.monitor.guest_request_userspace_enabled = 
> mop->u.guest_request.allow_userspace;
>  domain_unpause(d);
>  break;
>  }
> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index c10522b..de02507 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -396,15 +396,16 @@ struct arch_domain
>
>  /* Arch-specific monitor options */
>  struct {
> -unsigned int write_ctrlreg_enabled   : 4;
> -unsigned int write_ctrlreg_sync  : 4;
> -unsigned int write_ctrlreg_onchangeonly  : 4;
> -unsigned int singlestep_enabled  : 1;
> -unsigned int software_breakpoint_enabled : 1;
> -unsigned int debug_exception_enab

Re: [Xen-devel] [PATCH v5] x86/hvm: Allow guest_request vm_events coming from userspace

[Tamas K Lengyel]

On Wed, Aug 16, 2017 at 6:43 AM, Razvan Cojocaru
<rcojoc...@bitdefender.com> wrote:
> On 16.08.2017 15:32, Tamas K Lengyel wrote:
>>
>> On Wed, Aug 16, 2017 at 12:07 AM, Razvan Cojocaru
>> <rcojoc...@bitdefender.com> wrote:
>>>
>>> On 08/16/2017 02:16 AM, Tamas K Lengyel wrote:
>>>>
>>>> On Tue, Aug 15, 2017 at 2:06 AM, Jan Beulich <jbeul...@suse.com> wrote:
>>>>>>>>
>>>>>>>> On 14.08.17 at 17:53, <ta...@tklengyel.com> wrote:
>>>>>>
>>>>>> On Tue, Aug 8, 2017 at 2:27 AM, Alexandru Isaila
>>>>>> <aisa...@bitdefender.com> wrote:
>>>>>>>
>>>>>>> --- a/xen/arch/x86/hvm/hypercall.c
>>>>>>> +++ b/xen/arch/x86/hvm/hypercall.c
>>>>>>> @@ -155,6 +155,11 @@ int hvm_hypercall(struct cpu_user_regs *regs)
>>>>>>>   /* Fallthrough to permission check. */
>>>>>>>   case 4:
>>>>>>>   case 2:
>>>>>>> +if ( currd->arch.monitor.guest_request_userspace_enabled &&
>>>>>>> +eax == __HYPERVISOR_hvm_op &&
>>>>>>> +(mode == 8 ? regs->rdi : regs->ebx) ==
>>>>>>> HVMOP_guest_request_vm_event )
>>>>>>> +break;
>>>>>>> +
>>>>>>
>>>>>>
>>>>>> So the CPL check happens after the monitor check, which means this
>>>>>> will trigger regardless if the hypercall is coming from userspace or
>>>>>> kernelspace. Since the monitor option specifically says userspace,
>>>>>> this should probably get moved into the block where CPL was checked.
>>>>>
>>>>>
>>>>> What difference would this make? For CPL0 the hypercall is
>>>>> permitted anyway, and for CPL > 0 we specifically want to bypass
>>>>> the CPL check. Or are you saying you want to restrict the new
>>>>> check to just CPL3?
>>>>>
>>>>
>>>> Yes, according to the name of this monitor option this should only
>>>> trigger a vm_event when the hypercall is coming from CPL3. However,
>>>> the way it is implemented right now I see that this monitor option
>>>> actually requires the other one to be enabled too. By itself this
>>>> monitor option will not work. So I would also like to ask that the
>>>> check in xen/common/monitor.c, if ( d->monitor.guest_request_enabled
>>>> ), to be extended to be: if ( d->monitor.guest_request_enabled ||
>>>> d->monitor.guest_request_userspace_enabled )
>>>
>>>
>>> The option does not trigger anything. Its job is to allow guest requests
>>> coming from userspace (via VMCALLs). And not to _only_ allow these for
>>> userspace, but to allow them coming from userspace _as_well_.
>>>
>>> The current version of the patch, if I've not missed something, does not
>>> require d->monitor.guest_request_enabled to be true to work (the options
>>> can be toggled independently).
>>>
>>> The new function is meant to be called at any time, independent of
>>> enabling / disabling the guest request vm_event (i.e. it only controls
>>> its behaviour once it's enabled). So guest_request_userspace_enabled
>>> should not be used as synonym for guest_request_enabled.
>>>
>>
>> Hi Razvan,
>> so while monitor.guest_request_enabled can indeed be toggled
>> independently, if it is not set, how would the vm_event actually be
>> sent for monitor.guest_request_userspace_enabled? AFAICT it wouldn't
>> because the code responsible for sending the vm_event is gated only on
>> monitor.guest_request_enabled.
>
>
> Hello, indeed it wouldn't.
>
> This new option only control what happens _if_ monitor.guest_request_enabled
> is true. While monitor.guest_request_enabled is false, it has no effect. As
> soon as guest_request_enabled gets toggled on again, the behaviour will be
> the one that has been previously set by using
> guest_request_userspace_enabled.
>
>> And as for this new option being an extended version of
>> guest_request_enabled in the sense that it would allow both userspace
>> _and_ kernelspace, we can do that, but then the name of it is
>> misleading.
>
>
> The naming of the function did get shuffled around a bit. :)
>
> Would xc_monitor_allow_guest_userspace_event() be more appropriate?
>
> Also, if having a separate function feels counter-intuitive, we could also
> simply add a bool parameter to xc_monitor_guest_request(), for example:
>
> int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id,
>  bool enable, bool sync,
>  bool allow_userspace);
>
> and use that to toggle guest_request_userspace_enabled.

In light of the conversion here I have to say I like this option the most.

>
> Thanks,
> Razvan
>
> P.S. Is replying only to me (as opposed to a "reply all") intentional?

Oops, no, I've re-added xen-devel :)

Tamas

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

Re: [Xen-devel] [PATCH v5] x86/hvm: Allow guest_request vm_events coming from userspace

[Tamas K Lengyel]

On Tue, Aug 15, 2017 at 2:06 AM, Jan Beulich  wrote:
 On 14.08.17 at 17:53,  wrote:
>> On Tue, Aug 8, 2017 at 2:27 AM, Alexandru Isaila  
>> wrote:
>>> --- a/xen/arch/x86/hvm/hypercall.c
>>> +++ b/xen/arch/x86/hvm/hypercall.c
>>> @@ -155,6 +155,11 @@ int hvm_hypercall(struct cpu_user_regs *regs)
>>>  /* Fallthrough to permission check. */
>>>  case 4:
>>>  case 2:
>>> +if ( currd->arch.monitor.guest_request_userspace_enabled &&
>>> +eax == __HYPERVISOR_hvm_op &&
>>> +(mode == 8 ? regs->rdi : regs->ebx) == 
>>> HVMOP_guest_request_vm_event )
>>> +break;
>>> +
>>
>> So the CPL check happens after the monitor check, which means this
>> will trigger regardless if the hypercall is coming from userspace or
>> kernelspace. Since the monitor option specifically says userspace,
>> this should probably get moved into the block where CPL was checked.
>
> What difference would this make? For CPL0 the hypercall is
> permitted anyway, and for CPL > 0 we specifically want to bypass
> the CPL check. Or are you saying you want to restrict the new
> check to just CPL3?
>

Yes, according to the name of this monitor option this should only
trigger a vm_event when the hypercall is coming from CPL3. However,
the way it is implemented right now I see that this monitor option
actually requires the other one to be enabled too. By itself this
monitor option will not work. So I would also like to ask that the
check in xen/common/monitor.c, if ( d->monitor.guest_request_enabled
), to be extended to be: if ( d->monitor.guest_request_enabled ||
d->monitor.guest_request_userspace_enabled )

Tamas

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

Re: [Xen-devel] [PATCH v5] x86/hvm: Allow guest_request vm_events coming from userspace

[Tamas K Lengyel]

On Tue, Aug 8, 2017 at 2:27 AM, Alexandru Isaila
 wrote:
>
> In some introspection usecases, an in-guest agent needs to communicate
> with the external introspection agent.  An existing mechanism is
> HVMOP_guest_request_vm_event, but this is restricted to kernel usecases
> like all other hypercalls.
>
> Introduce a mechanism whereby the introspection agent can whitelist the
> use of HVMOP_guest_request_vm_event directly from userspace.
>
> Signed-off-by: Alexandru Isaila 
>
> ---
> Changes since V4:
> - Changed function mane from xc_allow_guest_userspace_event
>   to xc_monitor_guest_userspace_event
> - Fixed guest_request_enabled check
> - Delete the guest_request_sync
> - Changed guest_request_userspace_event to
>   guest_request_userspace_enabled
> - Moved guest_request_userspace_enabled flag from sched.h to
>   domain.h
> ---
>  tools/libxc/include/xenctrl.h |  1 +
>  tools/libxc/xc_monitor.c  | 14 ++
>  xen/arch/x86/hvm/hypercall.c  |  5 +
>  xen/common/monitor.c  | 13 +
>  xen/include/asm-x86/domain.h  | 19 ++-
>  xen/include/public/domctl.h   | 21 +++--
>  6 files changed, 54 insertions(+), 19 deletions(-)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index bde8313..c72e12d 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -2022,6 +2022,7 @@ int xc_monitor_descriptor_access(xc_interface *xch, 
> domid_t domain_id,
>   bool enable);
>  int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id,
>   bool enable, bool sync);
> +int xc_monitor_guest_userspace_event(xc_interface *xch, domid_t domain_id, 
> bool enable);
>  int xc_monitor_debug_exceptions(xc_interface *xch, domid_t domain_id,
>  bool enable, bool sync);
>  int xc_monitor_cpuid(xc_interface *xch, domid_t domain_id, bool enable);
> diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
> index b44ce93..bd8cbcf 100644
> --- a/tools/libxc/xc_monitor.c
> +++ b/tools/libxc/xc_monitor.c
> @@ -161,6 +161,20 @@ int xc_monitor_guest_request(xc_interface *xch, domid_t 
> domain_id, bool enable,
>  return do_domctl(xch, );
>  }
>
> +int xc_monitor_guest_userspace_event(xc_interface *xch, domid_t domain_id, 
> bool enable)
> +{
> +DECLARE_DOMCTL;
> +
> +domctl.cmd = XEN_DOMCTL_monitor_op;
> +domctl.domain = domain_id;
> +domctl.u.monitor_op.op = enable ? XEN_DOMCTL_MONITOR_OP_ENABLE
> +: XEN_DOMCTL_MONITOR_OP_DISABLE;
> +domctl.u.monitor_op.event = 
> XEN_DOMCTL_MONITOR_EVENT_GUEST_USERSPACE_EVENT;
> +
> +return do_domctl(xch, );
> +}
> +
> +
>  int xc_monitor_emulate_each_rep(xc_interface *xch, domid_t domain_id,
>  bool enable)
>  {
> diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
> index e7238ce..5742dd1 100644
> --- a/xen/arch/x86/hvm/hypercall.c
> +++ b/xen/arch/x86/hvm/hypercall.c
> @@ -155,6 +155,11 @@ int hvm_hypercall(struct cpu_user_regs *regs)
>  /* Fallthrough to permission check. */
>  case 4:
>  case 2:
> +if ( currd->arch.monitor.guest_request_userspace_enabled &&
> +eax == __HYPERVISOR_hvm_op &&
> +(mode == 8 ? regs->rdi : regs->ebx) == 
> HVMOP_guest_request_vm_event )
> +break;
> +

So the CPL check happens after the monitor check, which means this
will trigger regardless if the hypercall is coming from userspace or
kernelspace. Since the monitor option specifically says userspace,
this should probably get moved into the block where CPL was checked.

>  if ( unlikely(hvm_get_cpl(curr)) )
>  {
>  default:
> diff --git a/xen/common/monitor.c b/xen/common/monitor.c
> index 451f42f..080a405 100644
> --- a/xen/common/monitor.c
> +++ b/xen/common/monitor.c
> @@ -79,6 +79,19 @@ int monitor_domctl(struct domain *d, struct 
> xen_domctl_monitor_op *mop)
>  break;
>  }
>
> +case XEN_DOMCTL_MONITOR_EVENT_GUEST_USERSPACE_EVENT:
> +{
> +bool old_status = d->arch.monitor.guest_request_userspace_enabled;
> +
> +if ( unlikely(old_status == requested_status) )
> +return -EEXIST;
> +
> +domain_pause(d);
> +d->arch.monitor.guest_request_userspace_enabled = requested_status;
> +domain_unpause(d);
> +break;
> +}
> +
>  default:
>  /* Give arch-side the chance to handle this event */
>  return arch_monitor_domctl_event(d, mop);
> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index c10522b..de02507 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -396,15 +396,16 @@ struct arch_domain
>
>  /* Arch-specific monitor options */

Re: [Xen-devel] [PATCH v8 2/2] x86/monitor: Notify monitor if an emulation fails.

[Tamas K Lengyel]

On Tue, Aug 8, 2017 at 12:06 PM, Petre Pircalabu <ppircal...@bitdefender.com
> wrote:

> If case of a vm_event with the emulate_flags set, if the instruction
> is not implemented by the emulator, the monitor should be notified instead
> of directly injecting a hw exception.
> This behavior can be used to re-execute an instruction not supported by
> the emulator using the real processor (e.g. altp2m) instead of just
> crashing.
>
> Signed-off-by: Petre Pircalabu <ppircal...@bitdefender.com>
>

Acked-by: Tamas K Lengyel <ta...@tklengyel.com>
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4] x86/hvm: Allow guest_request vm_events coming from userspace

[Tamas K Lengyel]

On Sat, Aug 5, 2017 at 2:18 AM, Razvan Cojocaru <rcojoc...@bitdefender.com>
wrote:

> On 08/05/2017 04:32 AM, Tamas K Lengyel wrote:
> >
> >
> > On Fri, Aug 4, 2017 at 5:32 AM, Alexandru Isaila
> > <aisa...@bitdefender.com <mailto:aisa...@bitdefender.com>> wrote:
> >
> > In some introspection usecases, an in-guest agent needs to
> communicate
> > with the external introspection agent.  An existing mechanism is
> > HVMOP_guest_request_vm_event, but this is restricted to kernel
> usecases
> > like all other hypercalls.
> >
> > Introduce a mechanism whereby the introspection agent can whitelist
> the
> > use of HVMOP_guest_request_vm_event directly from userspace.
> >
> > Signed-off-by: Alexandru Isaila <aisa...@bitdefender.com
> > <mailto:aisa...@bitdefender.com>>
> >
> > ---
> > Changes since V3:
> > - Changed commit message
> > - Added new lines
> > - Indent the maximum space on the defines
> > - Chaned the name of the define/function name/struct member
> >   from vmcall to event
> > ---
> >  tools/libxc/include/xenctrl.h |  1 +
> >  tools/libxc/xc_monitor.c  | 14 ++
> >  xen/arch/x86/hvm/hypercall.c  |  5 +
> >  xen/common/monitor.c  | 14 ++
> >  xen/include/public/domctl.h   | 21 +++--
> >  xen/include/xen/sched.h   |  5 +++--
> >  6 files changed, 48 insertions(+), 12 deletions(-)
> >
> > diff --git a/tools/libxc/include/xenctrl.h
> > b/tools/libxc/include/xenctrl.h
> > index bde8313..90a056f 100644
> > --- a/tools/libxc/include/xenctrl.h
> > +++ b/tools/libxc/include/xenctrl.h
> > @@ -2022,6 +2022,7 @@ int xc_monitor_descriptor_access(xc_interface
> > *xch, domid_t domain_id,
> >   bool enable);
> >  int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id,
> >   bool enable, bool sync);
> > +int xc_allow_guest_userspace_event(xc_interface *xch, domid_t
> > domain_id, bool enable);
> >  int xc_monitor_debug_exceptions(xc_interface *xch, domid_t
> domain_id,
> >  bool enable, bool sync);
> >  int xc_monitor_cpuid(xc_interface *xch, domid_t domain_id, bool
> > enable);
> > diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
> > index b44ce93..6064c39 100644
> > --- a/tools/libxc/xc_monitor.c
> > +++ b/tools/libxc/xc_monitor.c
> > @@ -161,6 +161,20 @@ int xc_monitor_guest_request(xc_interface *xch,
> > domid_t domain_id, bool enable,
> >  return do_domctl(xch, );
> >  }
> >
> > +int xc_allow_guest_userspace_event(xc_interface *xch, domid_t
> > domain_id, bool enable)
> >
> >
> > This function should be prefixed with "xc_monitor_" like all the rest of
> > the functions here.
> That one was my suggestion, as I thought xc_monitor_-prefixed functions
> are meant to toggle monitoring somehow, whereas this function only
> toggles userspace use of guest request VMCALLs.
>

So it wasn't exactly clear whether this is just an option on the
pre-existing guest request monitor like sync or a completely new, separate
monitor option on its own. It looks to me like it is a separate option so
let's treat it as such.

Tamas
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4] x86/hvm: Allow guest_request vm_events coming from userspace

[Tamas K Lengyel]

On Fri, Aug 4, 2017 at 5:32 AM, Alexandru Isaila 
wrote:

> In some introspection usecases, an in-guest agent needs to communicate
> with the external introspection agent.  An existing mechanism is
> HVMOP_guest_request_vm_event, but this is restricted to kernel usecases
> like all other hypercalls.
>
> Introduce a mechanism whereby the introspection agent can whitelist the
> use of HVMOP_guest_request_vm_event directly from userspace.
>
> Signed-off-by: Alexandru Isaila 
>
> ---
> Changes since V3:
> - Changed commit message
> - Added new lines
> - Indent the maximum space on the defines
> - Chaned the name of the define/function name/struct member
>   from vmcall to event
> ---
>  tools/libxc/include/xenctrl.h |  1 +
>  tools/libxc/xc_monitor.c  | 14 ++
>  xen/arch/x86/hvm/hypercall.c  |  5 +
>  xen/common/monitor.c  | 14 ++
>  xen/include/public/domctl.h   | 21 +++--
>  xen/include/xen/sched.h   |  5 +++--
>  6 files changed, 48 insertions(+), 12 deletions(-)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index bde8313..90a056f 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -2022,6 +2022,7 @@ int xc_monitor_descriptor_access(xc_interface *xch,
> domid_t domain_id,
>   bool enable);
>  int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id,
>   bool enable, bool sync);
> +int xc_allow_guest_userspace_event(xc_interface *xch, domid_t domain_id,
> bool enable);
>  int xc_monitor_debug_exceptions(xc_interface *xch, domid_t domain_id,
>  bool enable, bool sync);
>  int xc_monitor_cpuid(xc_interface *xch, domid_t domain_id, bool enable);
> diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
> index b44ce93..6064c39 100644
> --- a/tools/libxc/xc_monitor.c
> +++ b/tools/libxc/xc_monitor.c
> @@ -161,6 +161,20 @@ int xc_monitor_guest_request(xc_interface *xch,
> domid_t domain_id, bool enable,
>  return do_domctl(xch, );
>  }
>
> +int xc_allow_guest_userspace_event(xc_interface *xch, domid_t domain_id,
> bool enable)
>

This function should be prefixed with "xc_monitor_" like all the rest of
the functions here.


> +{
> +DECLARE_DOMCTL;
> +
> +domctl.cmd = XEN_DOMCTL_monitor_op;
> +domctl.domain = domain_id;
> +domctl.u.monitor_op.op = enable ? XEN_DOMCTL_MONITOR_OP_ENABLE
> +: XEN_DOMCTL_MONITOR_OP_DISABLE;
> +domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_GUEST
> _USERSPACE_EVENT;
> +
> +return do_domctl(xch, );
> +}
> +
> +
>  int xc_monitor_emulate_each_rep(xc_interface *xch, domid_t domain_id,
>  bool enable)
>  {
> diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
> index e7238ce..8eb5f49 100644
> --- a/xen/arch/x86/hvm/hypercall.c
> +++ b/xen/arch/x86/hvm/hypercall.c
> @@ -155,6 +155,11 @@ int hvm_hypercall(struct cpu_user_regs *regs)
>  /* Fallthrough to permission check. */
>  case 4:
>  case 2:
> +if ( currd->monitor.guest_request_userspace_event &&

+eax == __HYPERVISOR_hvm_op &&
> +(mode == 8 ? regs->rdi : regs->ebx) ==
> HVMOP_guest_request_vm_event )
> +break;
> +
>  if ( unlikely(hvm_get_cpl(curr)) )
>  {
>  default:
> diff --git a/xen/common/monitor.c b/xen/common/monitor.c
> index 451f42f..21a1457 100644
> --- a/xen/common/monitor.c
> +++ b/xen/common/monitor.c
> @@ -79,6 +79,20 @@ int monitor_domctl(struct domain *d, struct
> xen_domctl_monitor_op *mop)
>  break;
>  }
>
> +case XEN_DOMCTL_MONITOR_EVENT_GUEST_USERSPACE_EVENT:
> +{
> +bool_t old_status = d->monitor.guest_request_enabled;
>

You are checking guest_request enabled here while later setting
guest_request_userspace_event. These are two separate monitor options,
adjust accordingly.


> +
> +if ( unlikely(old_status == requested_status) )
> +return -EEXIST;
> +
> +domain_pause(d);
> +d->monitor.guest_request_sync = mop->u.guest_request.sync;
>

You are setting guest_request_sync here which is a setting belonging to
guest_request_enabled. If you need sync/async option for the userspace
guest request it should be a separate bit. Since the toolstack side you add
in this patch never sets the sync option I assume that is not the case, so
remove this.


> +d->monitor.guest_request_userspace_event = requested_status;
> +domain_unpause(d);
> +break;
> +}
> +
>  default:
>  /* Give arch-side the chance to handle this event */
>  return arch_monitor_domctl_event(d, mop);
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index ff39762..870495c 100644
> --- 

Re: [Xen-devel] [PATCH v2] x86/hvm: Allow guest_request vm_events coming from userspace

[Tamas K Lengyel]

On Tue, Aug 1, 2017 at 4:30 AM, Andrew Cooper  wrote:
> On 01/08/17 10:46, Alexandru Isaila wrote:
>> Allow guest userspace code to request that a vm_event be sent out
>> via VMCALL. This functionality seems to be handy for a number of
>> Xen developers, as stated on the mailing list (thread "[Xen-devel]
>> HVMOP_guest_request_vm_event only works from guest in ring0").
>> This is a use case in communication between a userspace application
>> in the guest and the introspection application in dom0.
>>
>> Signed-off-by: Alexandru Isaila 
>
> This issue has been argued several times before, and while I am in
> favour of the change, there is a legitimate argument that it breaks one
> of our security boundaries.
>
> One intermediate option comes to mind however.
>
> Could we introduce a new monitor op which permits the use of
> HVMOP_guest_request_vm_event from userspace?  This way, it requires a
> positive action on behalf of the introspection agent to relax the CPL
> check, rather than having the CPL check unconditionally relaxed.

I agree, it would be required to gate this on a monitor option that is
disabled by default.

Tamas

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

[Xen-devel] Question about hvm_monitor_interrupt

[Tamas K Lengyel]

Hey Razvan,
the vm_event that is being generated by doing
VM_EVENT_FLAG_GET_NEXT_INTERRUPT sends almost all required information
about the interrupt to the listener to allow it to get reinjected,
except the instruction length. If the listener wants to reinject the
interrupt to the guest via xc_hvm_inject_trap the instruction length
is something needing to be specified. So shouldn't that information be
included in the vm_event?

Thanks,
Tamas

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

Re: [Xen-devel] [PATCH v3 1/2] x86/mm: Change default value for suppress #VE in set_mem_access()

[Tamas K Lengyel]

On Thu, Jul 20, 2017 at 12:25 PM, Razvan Cojocaru
<rcojoc...@bitdefender.com> wrote:
> On 07/20/2017 07:46 PM, Tamas K Lengyel wrote:
>> On Thu, Jul 20, 2017 at 10:43 AM, George Dunlap
>> <george.dun...@eu.citrix.com> wrote:
>>> On Wed, Jul 19, 2017 at 7:24 PM, Tamas K Lengyel <ta...@tklengyel.com> 
>>> wrote:
>>>>> I think the issue would be whether to allow a domain to set/clear the
>>>>> suppress #VE bit for its pages by calling the new HVMOP on itself.
>>>>
>>>> This problem is not limited to setting the SVE bit. It also applies to
>>>> swapping altp2m views. Pretty much all altp2m HVMOPs can be issued
>>>> from a user-space program without any way to check whether that
>>>> process is allowed to do that or not. If you don't think it is safe
>>>> for a domain to set SVE, the none of the altp2m ops are safe for the
>>>> domain to issue on itself. If we could say ensure only the kernel can
>>>> issue the hvmops, that would be OK. But that's not possible at the
>>>> moment AFAICT.
>>>
>>> Wait, is that right?  I think we normally restrict hypercalls to only
>>> being made from the guest kernel, don't we?
>>>
>>
>> If that's the case then it's good to know (can you point me where that
>> restriction is done?) I was just referring to the fact that
>> technically a userspace program can issue VMCALL.
>
> It is the case AFAIK. We had to do this trick to allow guest request
> hypercalls coming from guest userspace:
>
> https://github.com/xenserver/xen-4.7.pg/blob/master/master/xen-x86-hvm-Allow-guest_request-vm_events-coming-from-us.patch
>
> By default they can only come from the guest kernel.

Makes sense. Any reason not to have that patch upstreamed?

Tamas

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

Re: [Xen-devel] [PATCH v3 1/2] x86/mm: Change default value for suppress #VE in set_mem_access()

[Tamas K Lengyel]

On Thu, Jul 20, 2017 at 11:03 AM, Tamas K Lengyel <ta...@tklengyel.com> wrote:
> On Thu, Jul 20, 2017 at 10:57 AM, George Dunlap
> <george.dun...@citrix.com> wrote:
>> On 07/20/2017 05:46 PM, Tamas K Lengyel wrote:
>>> On Thu, Jul 20, 2017 at 10:43 AM, George Dunlap
>>> <george.dun...@eu.citrix.com> wrote:
>>>> On Wed, Jul 19, 2017 at 7:24 PM, Tamas K Lengyel <ta...@tklengyel.com> 
>>>> wrote:
>>>>>> I think the issue would be whether to allow a domain to set/clear the
>>>>>> suppress #VE bit for its pages by calling the new HVMOP on itself.
>>>>>
>>>>> This problem is not limited to setting the SVE bit. It also applies to
>>>>> swapping altp2m views. Pretty much all altp2m HVMOPs can be issued
>>>>> from a user-space program without any way to check whether that
>>>>> process is allowed to do that or not. If you don't think it is safe
>>>>> for a domain to set SVE, the none of the altp2m ops are safe for the
>>>>> domain to issue on itself. If we could say ensure only the kernel can
>>>>> issue the hvmops, that would be OK. But that's not possible at the
>>>>> moment AFAICT.
>>>>
>>>> Wait, is that right?  I think we normally restrict hypercalls to only
>>>> being made from the guest kernel, don't we?
>>>>
>>>
>>> If that's the case then it's good to know (can you point me where that
>>> restriction is done?) I was just referring to the fact that
>>> technically a userspace program can issue VMCALL.
>>
>> Well for vmcall in particular, it's in
>> xen/arch/x86/hvm/hypercall/hvm_hypercall().  The check for PV guests is
>> in xen/arch/x86/x86_64/entry.S:lstar_enter.  Other checks are left as an
>> exercise for the reader. :-)
>
> Thanks ;) I'm looking through it.

All checks out, we can ignore my concerns above. (Some comments around
vmx_get_cpl would have been helpful, took me a bit to find in the
manual why the bitshift is needed)

Tamas

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

Re: [Xen-devel] [PATCH v3 1/2] x86/mm: Change default value for suppress #VE in set_mem_access()

[Tamas K Lengyel]

On Thu, Jul 20, 2017 at 10:57 AM, George Dunlap
<george.dun...@citrix.com> wrote:
> On 07/20/2017 05:46 PM, Tamas K Lengyel wrote:
>> On Thu, Jul 20, 2017 at 10:43 AM, George Dunlap
>> <george.dun...@eu.citrix.com> wrote:
>>> On Wed, Jul 19, 2017 at 7:24 PM, Tamas K Lengyel <ta...@tklengyel.com> 
>>> wrote:
>>>>> I think the issue would be whether to allow a domain to set/clear the
>>>>> suppress #VE bit for its pages by calling the new HVMOP on itself.
>>>>
>>>> This problem is not limited to setting the SVE bit. It also applies to
>>>> swapping altp2m views. Pretty much all altp2m HVMOPs can be issued
>>>> from a user-space program without any way to check whether that
>>>> process is allowed to do that or not. If you don't think it is safe
>>>> for a domain to set SVE, the none of the altp2m ops are safe for the
>>>> domain to issue on itself. If we could say ensure only the kernel can
>>>> issue the hvmops, that would be OK. But that's not possible at the
>>>> moment AFAICT.
>>>
>>> Wait, is that right?  I think we normally restrict hypercalls to only
>>> being made from the guest kernel, don't we?
>>>
>>
>> If that's the case then it's good to know (can you point me where that
>> restriction is done?) I was just referring to the fact that
>> technically a userspace program can issue VMCALL.
>
> Well for vmcall in particular, it's in
> xen/arch/x86/hvm/hypercall/hvm_hypercall().  The check for PV guests is
> in xen/arch/x86/x86_64/entry.S:lstar_enter.  Other checks are left as an
> exercise for the reader. :-)

Thanks ;) I'm looking through it.

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

Re: [Xen-devel] [PATCH v3 1/2] x86/mm: Change default value for suppress #VE in set_mem_access()

[Tamas K Lengyel]

On Thu, Jul 20, 2017 at 10:43 AM, George Dunlap
<george.dun...@eu.citrix.com> wrote:
> On Wed, Jul 19, 2017 at 7:24 PM, Tamas K Lengyel <ta...@tklengyel.com> wrote:
>>> I think the issue would be whether to allow a domain to set/clear the
>>> suppress #VE bit for its pages by calling the new HVMOP on itself.
>>
>> This problem is not limited to setting the SVE bit. It also applies to
>> swapping altp2m views. Pretty much all altp2m HVMOPs can be issued
>> from a user-space program without any way to check whether that
>> process is allowed to do that or not. If you don't think it is safe
>> for a domain to set SVE, the none of the altp2m ops are safe for the
>> domain to issue on itself. If we could say ensure only the kernel can
>> issue the hvmops, that would be OK. But that's not possible at the
>> moment AFAICT.
>
> Wait, is that right?  I think we normally restrict hypercalls to only
> being made from the guest kernel, don't we?
>

If that's the case then it's good to know (can you point me where that
restriction is done?) I was just referring to the fact that
technically a userspace program can issue VMCALL.

Tamas

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

Re: [Xen-devel] [PATCH 2/2] x86/altp2m: Add a hvmop for setting the suppress #VE bit

[Tamas K Lengyel]

On Thu, Jul 20, 2017 at 9:11 AM, George Dunlap
<george.dun...@eu.citrix.com> wrote:
> On Thu, Jun 15, 2017 at 8:01 PM, Tamas K Lengyel <ta...@tklengyel.com> wrote:
>> On Fri, Jun 9, 2017 at 10:51 AM, Adrian Pop <a...@bitdefender.com> wrote:
>>> Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a
>>> privileged domain to change the value of the #VE suppress bit for a
>>> page.
>>>
>>> Add a libxc wrapper for invoking this hvmop.
>>>
>>> Signed-off-by: Adrian Pop <a...@bitdefender.com>
>>> ---
>>>  tools/libxc/include/xenctrl.h   |  2 ++
>>>  tools/libxc/xc_altp2m.c | 24 +++
>>>  xen/arch/x86/hvm/hvm.c  | 14 +++
>>>  xen/arch/x86/mm/mem_access.c| 52 
>>> +
>>>  xen/include/public/hvm/hvm_op.h | 15 
>>>  xen/include/xen/mem_access.h|  3 +++
>>>  6 files changed, 110 insertions(+)
>>>
>>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>>> index 1629f412dd..f6ba8635bf 100644
>>> --- a/tools/libxc/include/xenctrl.h
>>> +++ b/tools/libxc/include/xenctrl.h
>>> @@ -1926,6 +1926,8 @@ int xc_altp2m_destroy_view(xc_interface *handle, 
>>> domid_t domid,
>>>  /* Switch all vCPUs of the domain to the specified altp2m view */
>>>  int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
>>>   uint16_t view_id);
>>> +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid,
>>> +  uint16_t view_id, xen_pfn_t gfn, bool sve);
>>>  int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
>>>   uint16_t view_id, xen_pfn_t gfn,
>>>   xenmem_access_t access);
>>> diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
>>> index 0639632477..4710133918 100644
>>> --- a/tools/libxc/xc_altp2m.c
>>> +++ b/tools/libxc/xc_altp2m.c
>>> @@ -163,6 +163,30 @@ int xc_altp2m_switch_to_view(xc_interface *handle, 
>>> domid_t domid,
>>>  return rc;
>>>  }
>>>
>>> +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid,
>>> +  uint16_t view_id, xen_pfn_t gfn, bool sve)
>>> +{
>>> +int rc;
>>> +DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
>>> +
>>> +arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg));
>>> +if ( arg == NULL )
>>> +return -1;
>>> +
>>> +arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
>>> +arg->cmd = HVMOP_altp2m_set_suppress_ve;
>>> +arg->domain = domid;
>>> +arg->u.set_suppress_ve.view = view_id;
>>> +arg->u.set_suppress_ve.gfn = gfn;
>>> +arg->u.set_suppress_ve.suppress_ve = sve;
>>> +
>>> +rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
>>> + HYPERCALL_BUFFER_AS_ARG(arg));
>>> +
>>> +xc_hypercall_buffer_free(handle, arg);
>>> +return rc;
>>> +}
>>> +
>>>  int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
>>>   uint16_t view_id, xen_pfn_t gfn,
>>>   xenmem_access_t access)
>>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>>> index 70ddc81d44..dd8e205551 100644
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -4358,6 +4358,7 @@ static int do_altp2m_op(
>>>  case HVMOP_altp2m_destroy_p2m:
>>>  case HVMOP_altp2m_switch_p2m:
>>>  case HVMOP_altp2m_set_mem_access:
>>> +case HVMOP_altp2m_set_suppress_ve:
>>>  case HVMOP_altp2m_change_gfn:
>>>  break;
>>>  default:
>>> @@ -4475,6 +4476,19 @@ static int do_altp2m_op(
>>>  a.u.set_mem_access.view);
>>>  break;
>>>
>>> +case HVMOP_altp2m_set_suppress_ve:
>>> +if ( a.u.set_suppress_ve.pad1 || a.u.set_suppress_ve.pad2 )
>>> +rc = -EINVAL;
>>> +else
>>> +{
>>> +gfn_t gfn = _gfn(a.u.set_mem_access.gfn);
>>> +unsigned int altp2m_idx = a.u.set_mem_access.view;
>>> +bool suppress_ve = a.u.set_suppress_ve.suppress_ve;
>>> +
>>

Re: [Xen-devel] [PATCH 1/2] x86/mm: Change default value for suppress #VE in set_mem_access()

[Tamas K Lengyel]

On Thu, Jul 20, 2017 at 8:38 AM, George Dunlap
<george.dun...@eu.citrix.com> wrote:
> On Thu, Jun 15, 2017 at 7:49 PM, Tamas K Lengyel <ta...@tklengyel.com> wrote:
>> On Fri, Jun 9, 2017 at 10:51 AM, Adrian Pop <a...@bitdefender.com> wrote:
>>> From: Vlad Ioan Topan <ito...@bitdefender.com>
>>>
>>> The default value for the "suppress #VE" bit set by set_mem_access()
>>> currently depends on whether the call is made from the same domain (the
>>> bit is set when called from another domain and cleared if called from
>>> the same domain). This patch changes that behavior to inherit the old
>>> suppress #VE bit value if it is already set and to set it to 1
>>> otherwise, which is safer and more reliable.
>>
>> Could you elaborate on why do you think it is safer and more reliable
>> to switch the behavior? I believe the original idea was that the
>> domain should only be allowed to clear an SVE bit set by an external
>> tool. With this change it will allow the guest to request VE for any
>> page the external tool hasn't itself reserved specifically.
>
> Hmm?  This patch by itself simply prevents the guest from changing the
> VE bit at all (either setting or clearing it).
>
> Or did you mean, "This patch series"?

No, technically the other patch is fine by itself. It can only be used
to set the SVE bit from a privileged domain, but by itself that is
fine. Only this patch is problematic if we want to allow a setup where
there is only an in-guest tool without a corresponding vm_event
mem_access listener.

Tamas

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

Re: [Xen-devel] [PATCH v3 1/2] x86/mm: Change default value for suppress #VE in set_mem_access()

[Tamas K Lengyel]

On Wed, Jul 19, 2017 at 5:47 AM, Adrian Pop <a...@bitdefender.com> wrote:
> Hello,
>
> On Tue, Jul 18, 2017 at 11:26:45AM -0600, Tamas K Lengyel wrote:
>> On Tue, Jul 18, 2017 at 9:25 AM, Adrian Pop <a...@bitdefender.com> wrote:
>> > From: Vlad Ioan Topan <ito...@bitdefender.com>
>> >
>> > The default value for the "suppress #VE" bit set by set_mem_access()
>> > currently depends on whether the call is made from the same domain (the
>> > bit is set when called from another domain and cleared if called from
>> > the same domain). This patch changes that behavior to inherit the old
>> > suppress #VE bit value if it is already set and to set it to 1
>> > otherwise, which is safer and more reliable.
>>
>> With the way things are currently if the in-guest tool calls
>> set_mem_access for an altp2m view, it implies it wants to receive #VE
>> for it. Wouldn't this change in this patch effectively make it
>> impossible for an in-guest tool to decide which pages it wants to
>> receive #VE for? The new HVMOP you are introducing is only accessible
>> from a privileged domain..
>
> Yes, this change, along with the restrictions from the new HVMOP would
> virtually prevent a guest from changing the suppress #VE bit for its
> pages. The current set_mem_access functionality, if I'm not mistaken,
> is a bit odd since the guest can only clear the sve, but to set it,
> another domain would have to call set_mem_access for it.

Stating that change explicitly in the patch message would have been
something I would want to see.

Calling set_mem_access from the guest itself by design clears the SVE
bit, which makes sense. The in-guest tool doesn't know whether there
is an external mem_access listener, so the only thing it should be
allowed to do is to signal to the hypervisor that when it changes EPT
permissions, violations on those pages need to be injected into the
guest with #VE. If you don't want to allow a domain to make changes
like that, you need to restrict altp2m ops to be issued from the
domain completely.

>
> I think the issue would be whether to allow a domain to set/clear the
> suppress #VE bit for its pages by calling the new HVMOP on itself.

This problem is not limited to setting the SVE bit. It also applies to
swapping altp2m views. Pretty much all altp2m HVMOPs can be issued
from a user-space program without any way to check whether that
process is allowed to do that or not. If you don't think it is safe
for a domain to set SVE, the none of the altp2m ops are safe for the
domain to issue on itself. If we could say ensure only the kernel can
issue the hvmops, that would be OK. But that's not possible at the
moment AFAICT.

Tamas

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

Re: [Xen-devel] [PATCH v3 1/2] x86/mm: Change default value for suppress #VE in set_mem_access()

[Tamas K Lengyel]

On Tue, Jul 18, 2017 at 9:25 AM, Adrian Pop  wrote:
> From: Vlad Ioan Topan 
>
> The default value for the "suppress #VE" bit set by set_mem_access()
> currently depends on whether the call is made from the same domain (the
> bit is set when called from another domain and cleared if called from
> the same domain). This patch changes that behavior to inherit the old
> suppress #VE bit value if it is already set and to set it to 1
> otherwise, which is safer and more reliable.

With the way things are currently if the in-guest tool calls
set_mem_access for an altp2m view, it implies it wants to receive #VE
for it. Wouldn't this change in this patch effectively make it
impossible for an in-guest tool to decide which pages it wants to
receive #VE for? The new HVMOP you are introducing is only accessible
from a privileged domain..

Tamas

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

Re: [Xen-devel] [PATCH v3 2/2] x86/altp2m: Add a hvmop for setting the suppress #VE bit

[Tamas K Lengyel]

On Tue, Jul 18, 2017 at 9:25 AM, Adrian Pop <a...@bitdefender.com> wrote:
> Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a
> privileged domain to change the value of the #VE suppress bit for a
> page.
>
> Add a libxc wrapper for invoking this hvmop.
>
> Signed-off-by: Adrian Pop <a...@bitdefender.com>
> Acked-by: Wei Liu <wei.l...@citrix.com>

Acked-by: Tamas K Lengyel <ta...@tklengyel.com>

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

Re: [Xen-devel] [PATCH v6] x86/monitor: Notify monitor if an emulation fails.

[Tamas K Lengyel]

On Tue, Jul 18, 2017 at 3:37 AM, Petre Pircalabu
<ppircal...@bitdefender.com> wrote:
> If case of a vm_event with the emulate_flags set, if the instruction
> cannot be emulated, the monitor should be notified instead of directly
> injecting a hw exception.
> This behavior can be used to re-execute an instruction not supported by
> the emulator using the real processor (e.g. altp2m) instead of just
> crashing.
>
> Signed-off-by: Petre Pircalabu <ppircal...@bitdefender.com>

Acked-by: Tamas K Lengyel <ta...@tklengyel.com>

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

Re: [Xen-devel] [PATCH v4] x86/monitor: Notify monitor if an emulation fails.

[Tamas K Lengyel]

On Wed, Jul 12, 2017 at 11:21 AM, Petre Pircalabu
 wrote:
> If case of a vm_event with the emulate_flags set, if the instruction
> cannot be emulated, the monitor should be notified instead of directly
> injecting a hw exception.
> This behavior can be used to re-execute an instruction not supported by
> the emulator using the real processor (e.g. altp2m) instead of just
> crashing.
>
> Signed-off-by: Petre Pircalabu 
>
> ---
> Changed since v1:
>   * Removed the emulation kind check when calling hvm_inject_hw_exception
>
> Changed since v2:
>   * Removed a file added by mistake
>
> Changed since v3:
>   * Removed extra stray line
>   * Added the _enabled suffix to the emul_unhandleable monitor option
> ---
>  tools/libxc/include/xenctrl.h |  2 ++
>  tools/libxc/xc_monitor.c  | 14 ++
>  xen/arch/x86/hvm/emulate.c|  5 -
>  xen/arch/x86/hvm/monitor.c| 18 ++
>  xen/arch/x86/monitor.c| 12 
>  xen/include/asm-x86/domain.h  |  1 +
>  xen/include/asm-x86/hvm/monitor.h |  1 +
>  xen/include/asm-x86/monitor.h |  3 ++-
>  xen/include/public/domctl.h   |  1 +
>  xen/include/public/vm_event.h |  2 ++
>  10 files changed, 57 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index c51bb3b..8deb5ac 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -2029,6 +2029,8 @@ int xc_monitor_debug_exceptions(xc_interface *xch, 
> domid_t domain_id,
>  int xc_monitor_cpuid(xc_interface *xch, domid_t domain_id, bool enable);
>  int xc_monitor_privileged_call(xc_interface *xch, domid_t domain_id,
> bool enable);
> +int xc_monitor_emul_unhandleable(xc_interface *xch, domid_t domain_id,
> + bool enable);
>  /**
>   * This function enables / disables emulation for each REP for a
>   * REP-compatible instruction.
> diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
> index b44ce93..8e72c6c 100644
> --- a/tools/libxc/xc_monitor.c
> +++ b/tools/libxc/xc_monitor.c
> @@ -216,6 +216,20 @@ int xc_monitor_privileged_call(xc_interface *xch, 
> domid_t domain_id,
>  return do_domctl(xch, );
>  }
>
> +int xc_monitor_emul_unhandleable(xc_interface *xch, domid_t domain_id,
> + bool enable)
> +{
> +DECLARE_DOMCTL;
> +
> +domctl.cmd = XEN_DOMCTL_monitor_op;
> +domctl.domain = domain_id;
> +domctl.u.monitor_op.op = enable ? XEN_DOMCTL_MONITOR_OP_ENABLE
> +: XEN_DOMCTL_MONITOR_OP_DISABLE;
> +domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_EMUL_UNHANDLEABLE;
> +
> +return do_domctl(xch, );
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index e97aa69..f52aa87 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -14,12 +14,14 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -2101,7 +2103,8 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, 
> unsigned int trapnr,
>  return;
>  case X86EMUL_UNHANDLEABLE:
>  hvm_dump_emulation_state(XENLOG_G_DEBUG, "Mem event", );
> -hvm_inject_hw_exception(trapnr, errcode);
> +if ( !hvm_monitor_emul_unhandleable() )
> +hvm_inject_hw_exception(trapnr, errcode);
>  break;
>  case X86EMUL_EXCEPTION:
>  hvm_inject_event();
> diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c
> index a7ccfc4..977a96b 100644
> --- a/xen/arch/x86/hvm/monitor.c
> +++ b/xen/arch/x86/hvm/monitor.c
> @@ -57,6 +57,24 @@ bool_t hvm_monitor_cr(unsigned int index, unsigned long 
> value, unsigned long old
>  return 0;
>  }
>
> +bool hvm_monitor_emul_unhandleable(void)
> +{
> +struct vcpu *curr = current;
> +struct domain *d = curr->domain;
> +
> +/*
> + * Send a vm_event to the monitor to signal that the current
> + * instruction couldn't be emulated.
> + */
> +vm_event_request_t req = {
> +.reason = VM_EVENT_REASON_EMUL_UNHANDLEABLE,
> +.vcpu_id  = curr->vcpu_id,
> +};
> +
> +return ( d->arch.monitor.emul_unhandleable_enabled &&
> + monitor_traps(curr, true, ) );

So what happens if monitor_traps fails and returns -1? Since this gets
treated as a boolean, we will return true here and just silently
swallow the problem and likely make the VM hang in a loop. I think it
would be appropriate to crash the vm here if we can't emulate and also
can't notify the listener.

> +}
> +
>  void hvm_monitor_msr(unsigned int msr, uint64_t value)
>  {
>  struct vcpu *curr = current;
> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c

Re: [Xen-devel] [PATCH v3] x86/monitor: Notify monitor if an emulation fails.

[Tamas K Lengyel]

On Wed, Jul 12, 2017 at 2:43 AM, Petre Pircalabu
 wrote:
> If case of a vm_event with the emulate_flags set, if the instruction
> cannot be emulated, the monitor should be notified instead of directly
> injecting a hw exception.
> This behavior can be used to re-execute an instruction not supported by
> the emulator using the real processor (e.g. altp2m) instead of just
> crashing.
>
> Signed-off-by: Petre Pircalabu 
>
> ---
> Changed since v1:
>   * Removed the emulation kind check when calling hvm_inject_hw_exception
>
> Changed since v2:
>   * Removed a file added by mistake
> ---
>  tools/libxc/include/xenctrl.h |  2 ++
>  tools/libxc/xc_monitor.c  | 14 ++
>  xen/arch/x86/hvm/emulate.c|  5 -
>  xen/arch/x86/hvm/monitor.c| 19 +++
>  xen/arch/x86/monitor.c| 12 
>  xen/include/asm-x86/domain.h  |  1 +
>  xen/include/asm-x86/hvm/monitor.h |  1 +
>  xen/include/asm-x86/monitor.h |  3 ++-
>  xen/include/public/domctl.h   |  1 +
>  xen/include/public/vm_event.h |  2 ++
>  10 files changed, 58 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index c51bb3b..8deb5ac 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -2029,6 +2029,8 @@ int xc_monitor_debug_exceptions(xc_interface *xch, 
> domid_t domain_id,
>  int xc_monitor_cpuid(xc_interface *xch, domid_t domain_id, bool enable);
>  int xc_monitor_privileged_call(xc_interface *xch, domid_t domain_id,
> bool enable);
> +int xc_monitor_emul_unhandleable(xc_interface *xch, domid_t domain_id,
> + bool enable);
>  /**
>   * This function enables / disables emulation for each REP for a
>   * REP-compatible instruction.
> diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
> index b44ce93..8e72c6c 100644
> --- a/tools/libxc/xc_monitor.c
> +++ b/tools/libxc/xc_monitor.c
> @@ -216,6 +216,20 @@ int xc_monitor_privileged_call(xc_interface *xch, 
> domid_t domain_id,
>  return do_domctl(xch, );
>  }
>
> +int xc_monitor_emul_unhandleable(xc_interface *xch, domid_t domain_id,
> + bool enable)
> +{
> +DECLARE_DOMCTL;
> +
> +domctl.cmd = XEN_DOMCTL_monitor_op;
> +domctl.domain = domain_id;
> +domctl.u.monitor_op.op = enable ? XEN_DOMCTL_MONITOR_OP_ENABLE
> +: XEN_DOMCTL_MONITOR_OP_DISABLE;
> +domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_EMUL_UNHANDLEABLE;
> +
> +return do_domctl(xch, );
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index e97aa69..f52aa87 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -14,12 +14,14 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -2101,7 +2103,8 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, 
> unsigned int trapnr,
>  return;
>  case X86EMUL_UNHANDLEABLE:
>  hvm_dump_emulation_state(XENLOG_G_DEBUG, "Mem event", );
> -hvm_inject_hw_exception(trapnr, errcode);
> +if ( !hvm_monitor_emul_unhandleable() )
> +hvm_inject_hw_exception(trapnr, errcode);
>  break;
>  case X86EMUL_EXCEPTION:
>  hvm_inject_event();
> diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c
> index a7ccfc4..02e0ba5 100644
> --- a/xen/arch/x86/hvm/monitor.c
> +++ b/xen/arch/x86/hvm/monitor.c
> @@ -57,6 +57,25 @@ bool_t hvm_monitor_cr(unsigned int index, unsigned long 
> value, unsigned long old
>  return 0;
>  }
>
> +

Stray extra line here.

> +bool hvm_monitor_emul_unhandleable(void)
> +{
> +struct vcpu *curr = current;
> +struct domain *d = curr->domain;
> +
> +/*
> + * Send a vm_event to the monitor to signal that the current
> + * instruction couldn't be emulated.
> + */
> +vm_event_request_t req = {
> +.reason = VM_EVENT_REASON_EMUL_UNHANDLEABLE,
> +.vcpu_id  = curr->vcpu_id,
> +};
> +
> +return ( d->arch.monitor.emul_unhandleable &&
> + monitor_traps(curr, true, ) );
> +}
> +
>  void hvm_monitor_msr(unsigned int msr, uint64_t value)
>  {
>  struct vcpu *curr = current;
> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
> index 706454f..51252fe 100644
> --- a/xen/arch/x86/monitor.c
> +++ b/xen/arch/x86/monitor.c
> @@ -283,6 +283,18 @@ int arch_monitor_domctl_event(struct domain *d,
>  break;
>  }
>
> +case XEN_DOMCTL_MONITOR_EVENT_EMUL_UNHANDLEABLE:
> +{
> +bool old_status = ad->monitor.emul_unhandleable;
> +if ( unlikely(old_status == requested_status) )
> +return -EEXIST;
> 

Re: [Xen-devel] [PATCH v2] x86/monitor: Notify monitor if an emulation fails.

[Tamas K Lengyel]

On Tue, Jul 11, 2017 at 8:53 AM, Petre Pircalabu
 wrote:
> If case of a vm_event with the emulate_flags set, if the instruction
> cannot be emulated, the monitor should be notified instead of directly
> injecting a hw exception.
> This behavior can be used to re-execute an instruction not supported by
> the emulator using the real processor (e.g. altp2m) instead of just
> crashing.
>
> Signed-off-by: Petre Pircalabu 
>
> ---
> Changed since v1:
>   * Removed the emulation kind check when calling hvm_inject_hw_exception
> ---
>  tools/libxc/include/xenctrl.h  |   2 +
>  tools/libxc/xc_monitor.c   |  14 ++
>  ...itor-Notify-monitor-if-an-emulation-fails.patch | 212 
> +
>  xen/arch/x86/hvm/emulate.c |   5 +-
>  xen/arch/x86/hvm/monitor.c |  19 ++
>  xen/arch/x86/monitor.c |  12 ++
>  xen/include/asm-x86/domain.h   |   1 +
>  xen/include/asm-x86/hvm/monitor.h  |   1 +
>  xen/include/asm-x86/monitor.h  |   3 +-
>  xen/include/public/domctl.h|   1 +
>  xen/include/public/vm_event.h  |   2 +
>  11 files changed, 270 insertions(+), 2 deletions(-)
>  create mode 100644 
> tools/tests/xen-access/0001-x86-monitor-Notify-monitor-if-an-emulation-fails.patch

I don't think you meant to add this patch file.

Tamas

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

Re: [Xen-devel] [PATCH] x86/monitor: Notify monitor if an emulation fails.

[Tamas K Lengyel]

On Mon, Jul 10, 2017 at 11:07 AM, Petre Pircalabu
 wrote:
> If case of a vm_event with the emulate_flags set, if the instruction
> cannot be emulated, the monitor should be notified instead of directly
> injecting a hw exception.
> This behavior can be used to re-execute an instruction not supported by
> the emulator using the real processor (e.g. altp2m) instead of just
> crashing.
>
> Signed-off-by: Petre Pircalabu 
> ---
>  tools/libxc/include/xenctrl.h |  2 ++
>  tools/libxc/xc_monitor.c  | 14 ++
>  xen/arch/x86/hvm/emulate.c|  5 -
>  xen/arch/x86/hvm/monitor.c| 19 +++
>  xen/arch/x86/monitor.c| 12 
>  xen/include/asm-x86/domain.h  |  1 +
>  xen/include/asm-x86/hvm/monitor.h |  1 +
>  xen/include/asm-x86/monitor.h |  3 ++-
>  xen/include/public/domctl.h   |  1 +
>  xen/include/public/vm_event.h |  2 ++
>  10 files changed, 58 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index c51bb3b..8deb5ac 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -2029,6 +2029,8 @@ int xc_monitor_debug_exceptions(xc_interface *xch, 
> domid_t domain_id,
>  int xc_monitor_cpuid(xc_interface *xch, domid_t domain_id, bool enable);
>  int xc_monitor_privileged_call(xc_interface *xch, domid_t domain_id,
> bool enable);
> +int xc_monitor_emul_unhandleable(xc_interface *xch, domid_t domain_id,
> + bool enable);
>  /**
>   * This function enables / disables emulation for each REP for a
>   * REP-compatible instruction.
> diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
> index b44ce93..8e72c6c 100644
> --- a/tools/libxc/xc_monitor.c
> +++ b/tools/libxc/xc_monitor.c
> @@ -216,6 +216,20 @@ int xc_monitor_privileged_call(xc_interface *xch, 
> domid_t domain_id,
>  return do_domctl(xch, );
>  }
>
> +int xc_monitor_emul_unhandleable(xc_interface *xch, domid_t domain_id,
> + bool enable)
> +{
> +DECLARE_DOMCTL;
> +
> +domctl.cmd = XEN_DOMCTL_monitor_op;
> +domctl.domain = domain_id;
> +domctl.u.monitor_op.op = enable ? XEN_DOMCTL_MONITOR_OP_ENABLE
> +: XEN_DOMCTL_MONITOR_OP_DISABLE;
> +domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_EMUL_UNHANDLEABLE;
> +
> +return do_domctl(xch, );
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index e97aa69..083a38a 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -14,12 +14,14 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -2101,7 +2103,8 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, 
> unsigned int trapnr,
>  return;
>  case X86EMUL_UNHANDLEABLE:
>  hvm_dump_emulation_state(XENLOG_G_DEBUG, "Mem event", );
> -hvm_inject_hw_exception(trapnr, errcode);
> +if ( (kind != EMUL_KIND_NORMAL) || !hvm_monitor_emul_unhandleable() )

Why is there this check for !EMUL_KIND_NORMAL?

Tamas

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

Re: [Xen-devel] [For 4.9] Updating https://wiki.xenproject.org/wiki/Xen_Project_Release_Features to reflect support status of new features

[Tamas K Lengyel]

On Tue, Jun 27, 2017 at 3:48 AM, Razvan Cojocaru
 wrote:
> Hello,
>
>> - Security > Alternative 2pm : Supported – I think we should split this
>> out – it is currently implicitly covered under "Virtual Machine
>> Introspection"
>
> I agree that altp2m deserves its own space. While we're interested in
> it, our current solution makes no use of it, so it's certainly possible
> to do introspection without any help from altp2m.

I do use it with introspection but I think there were also use-cases
for it that were not introspection related. So I think splitting it
out is correct.

>
> From my, albeit limited, experience, altp2m probably fits under "Tech
> preview".

Sounds right to me.

>
> If we subtract altp2m from the general "Virtual Machine Introspection"
> category, I'd say that the upstream support is between "Tech Preview"
> and "Supported". I'll explain.
>
> The interface is largely stable, but we may need to add optimizations
> which might change it - so while we don't want this to be happening a
> lot, it will happen.

IMHO if we consider stuff like the domctl interface stable then the
vm_event interface can be considered stable too. I don't think stable
means that it doesn't change, I think it means that it changes in a
way that users can clearly build with it and/or detect the changes via
interface versioning, which we do have here as well.

>
> There's also functional stability with the XenServer patches (which
> bypass the emulator (single-step) when it can't emulate, and has a
> version of the old smp_lock() emulator race-condition patch).
> Unfortunately, upstream Xen still has race condition issues when
> emulating LOCKed instructions in SMP scenarios - this will be addressed
> ASAP with a proper CMPXCHG patch that currently depends on a patch from
> Andrew and will require rework of a patch from Jan that adds a specific
> emulation return code for CMPXCHG failures.
>
> There's also an issue with #UD injections which is covered by the
> emulator bypass patch in XenServer but can cause IPIs to get lost with
> the upstream code - that will also require some more thinking.
>
> So there are still (emulation-related) quirks upstream. We'd very much
> like to get them ironed out.
>

IMHO the emulator of Xen is not part of introspection. Just as with
altp2m, it can be used in that context, but it is not part of it.

Tamas

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

Re: [Xen-devel] [PATCH] common/vm_event: Initialize vm_event lists on domain creation

[Tamas K Lengyel]

On Tue, Jun 27, 2017 at 8:25 AM, Razvan Cojocaru
 wrote:
> On 06/27/2017 02:45 PM, Jan Beulich wrote:
> Razvan Cojocaru  06/27/17 1:38 PM >>>
>>> On 06/27/2017 02:26 PM, Jan Beulich wrote:
>>> Razvan Cojocaru  06/27/17 10:32 AM >>>
> On 06/27/2017 09:21 AM, Jan Beulich wrote:
> Andrew Cooper  06/26/17 5:11 PM >>>
>>> There is also a difference in timing; vm_event_init_domain() is called
>>> when vm_event is started on the domain, not when the domain is
>>> constructed.  IMO, the two should happen at the same time to be
>>> consistent.  I'm not fussed at which point, as it would be fine (in
>>> principle) for d->vm_event to be NULL in most cases.
>>
>> I very much support that last aspect - there's shouldn't be any memory
>> allocated here if the domain isn't going to make use of VM events.
>
> Unfortunately it's not as simple as that.
>
> While I didn't write that code, it would seem that on domain creation
> that data is being allocated because it holds information about 3
> different vm_event subsystems - monitor, paging, and memshare.

 But it can't be that difficult to make it work the suggested way?
>>>
>>> We can lazy-allocate that the first time any one of the three gets
>>> initialized (monitor, paging, share), and update all the code that
>>> checks d->vm_event->member to check that d->vm_event is not NULL before
>>> that.
>>>
>>> Any objections?
>>
>> None here.
>
> That's actually much more involved than I thought: almost every vm_event
> function assumes that for a created domain d->vm_event is not NULL, and
> takes a struct vm_event_domain *ved parameter to differentiate between
> monitor, mem paging and mem sharing. So while this technically is not a
> hard thing to fix at all, it would touch a lot of ARM and x86 code and
> be quite an overhaul of vm_event.

That sounds right, since currently if a domain is created it is
guaranteed to have d->vm_event allocated. That is quite a large struct
so I think it would be a nice optimization to only allocate it when
needed. If we are reworking this code, I would prefer to see that
structure actually being removed altogether and just have three
pointers to vm_event_domain's (monitor/sharing/paging), which get
allocated when needed. The pointers for paging and sharing could then
further be wrapped in ifdef CONFIG checks, so we really only have
memory for what we need to.

>
> My immediate reaction was to add small helper functions such as:
>
>  42 static struct vm_event_domain *vm_event_get_ved(
>  43 struct domain *d,
>  44 enum vm_event_domain_type domain_type)
>  45 {
>  46 if ( !d->vm_event )
>  47 return NULL;
>  48
>  49 switch ( domain_type )
>  50 {
>  51 #ifdef CONFIG_HAS_MEM_PAGING
>  52 case VM_EVENT_DOMAIN_TYPE_MEM_PAGING:
>  53 return >vm_event->paging;
>  54 #endif
>  55 case VM_EVENT_DOMAIN_TYPE_MONITOR:
>  56 return >vm_event->monitor;
>  57 #ifdef CONFIG_HAS_MEM_SHARING
>  58 case VM_EVENT_DOMAIN_TYPE_MEM_SHARING:
>  59 return >vm_event->share;
>  60 #endif
>  61 default:
>  62 return NULL;
>  63 }
>  64 }
>
> and try to get all places to use that line of thinking, but there's
> quite a lot of them.
>
> Tamas, any thoughts on this before going deeper?

Having this helper would be fine (even with my proposed changes
above). And yes, I can see this would involve touching a lot of code,
but I presume it will be mostly mechanical.

Tamas

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

Re: [Xen-devel] [PATCH] common/vm_event: Initialize vm_event lists on domain creation

[Tamas K Lengyel]

On Mon, Jun 26, 2017 at 9:09 AM, Andrew Cooper
<andrew.coop...@citrix.com> wrote:
> On 26/06/17 15:52, Tamas K Lengyel wrote:
>> On Mon, Jun 26, 2017 at 3:48 AM, Razvan Cojocaru
>> <rcojoc...@bitdefender.com> wrote:
>>> Pending livepatch code wants to check if the vm_event wait queues
>>> are active, and this is made harder by the fact that they were
>>> previously only initialized some time after the domain was created,
>>> in vm_event_enable(). This patch initializes the lists immediately
>>> after xzalloc()ating the vm_event memory, in domain_create(), in
>>> the newly added init_domain_vm_event() function.
>>>
>>> Signed-off-by: Razvan Cojocaru <rcojoc...@bitdefender.com>
>>> ---
>>>  xen/common/domain.c|  5 ++---
>>>  xen/common/vm_event.c  | 23 ---
>>>  xen/include/xen/vm_event.h |  2 ++
>>>  3 files changed, 24 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>>> index b22aacc..89a8f1d 100644
>>> --- a/xen/common/domain.c
>>> +++ b/xen/common/domain.c
>>> @@ -362,11 +362,10 @@ struct domain *domain_create(domid_t domid, unsigned 
>>> int domcr_flags,
>>>
>>>  poolid = 0;
>>>
>>> -err = -ENOMEM;
>>> -d->vm_event = xzalloc(struct vm_event_per_domain);
>>> -if ( !d->vm_event )
>>> +if ( (err = init_domain_vm_event(d)) != 0 )
>>>  goto fail;
>>>
>>> +err = -ENOMEM;
>>>  d->pbuf = xzalloc_array(char, DOMAIN_PBUF_SIZE);
>>>  if ( !d->pbuf )
>>>  goto fail;
>>> diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
>>> index 9291db6..294ddd7 100644
>>> --- a/xen/common/vm_event.c
>>> +++ b/xen/common/vm_event.c
>>> @@ -39,6 +39,26 @@
>>>  #define vm_event_ring_lock(_ved)   spin_lock(&(_ved)->ring_lock)
>>>  #define vm_event_ring_unlock(_ved) spin_unlock(&(_ved)->ring_lock)
>>>
>>> +int init_domain_vm_event(struct domain *d)
>> We already have a vm_event_init_domain function so the naming of this
>> one here is not a particularly good one. It also looks like to me
>> these two functions could simply be merged..
>
> They shouldn't be merged.
>
> The current vm_event_init_domain() should probably be
> init_domain_arch_vm_event(), as it deals with constructing
> d->arch.vm_event, whereas this function deals with d->vm_event.

That would be fine for me, it's really the naming that I had a problem with.

Thanks,
Tamas

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

Re: [Xen-devel] [PATCH] common/vm_event: Initialize vm_event lists on domain creation

[Tamas K Lengyel]

On Mon, Jun 26, 2017 at 3:48 AM, Razvan Cojocaru
 wrote:
> Pending livepatch code wants to check if the vm_event wait queues
> are active, and this is made harder by the fact that they were
> previously only initialized some time after the domain was created,
> in vm_event_enable(). This patch initializes the lists immediately
> after xzalloc()ating the vm_event memory, in domain_create(), in
> the newly added init_domain_vm_event() function.
>
> Signed-off-by: Razvan Cojocaru 
> ---
>  xen/common/domain.c|  5 ++---
>  xen/common/vm_event.c  | 23 ---
>  xen/include/xen/vm_event.h |  2 ++
>  3 files changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index b22aacc..89a8f1d 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -362,11 +362,10 @@ struct domain *domain_create(domid_t domid, unsigned 
> int domcr_flags,
>
>  poolid = 0;
>
> -err = -ENOMEM;
> -d->vm_event = xzalloc(struct vm_event_per_domain);
> -if ( !d->vm_event )
> +if ( (err = init_domain_vm_event(d)) != 0 )
>  goto fail;
>
> +err = -ENOMEM;
>  d->pbuf = xzalloc_array(char, DOMAIN_PBUF_SIZE);
>  if ( !d->pbuf )
>  goto fail;
> diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
> index 9291db6..294ddd7 100644
> --- a/xen/common/vm_event.c
> +++ b/xen/common/vm_event.c
> @@ -39,6 +39,26 @@
>  #define vm_event_ring_lock(_ved)   spin_lock(&(_ved)->ring_lock)
>  #define vm_event_ring_unlock(_ved) spin_unlock(&(_ved)->ring_lock)
>
> +int init_domain_vm_event(struct domain *d)

We already have a vm_event_init_domain function so the naming of this
one here is not a particularly good one. It also looks like to me
these two functions could simply be merged..

> +{
> +d->vm_event = xzalloc(struct vm_event_per_domain);
> +
> +if ( !d->vm_event )
> +return -ENOMEM;
> +
> +#ifdef CONFIG_HAS_MEM_PAGING
> +init_waitqueue_head(>vm_event->paging.wq);
> +#endif
> +
> +init_waitqueue_head(>vm_event->monitor.wq);

Move this one up before the #ifdef block for MEM_PAGING.

> +
> +#ifdef CONFIG_HAS_MEM_SHARING
> +init_waitqueue_head(>vm_event->share.wq);
> +#endif
> +
> +return 0;
> +}
> +
>  static int vm_event_enable(
>  struct domain *d,
>  xen_domctl_vm_event_op_t *vec,
> @@ -93,9 +113,6 @@ static int vm_event_enable(
>  /* Save the pause flag for this particular ring. */
>  ved->pause_flag = pause_flag;
>
> -/* Initialize the last-chance wait queue. */
> -init_waitqueue_head(>wq);
> -
>  vm_event_ring_unlock(ved);
>  return 0;
>
> diff --git a/xen/include/xen/vm_event.h b/xen/include/xen/vm_event.h
> index 2fb3951..482243e 100644
> --- a/xen/include/xen/vm_event.h
> +++ b/xen/include/xen/vm_event.h
> @@ -80,6 +80,8 @@ void vm_event_set_registers(struct vcpu *v, 
> vm_event_response_t *rsp);
>
>  void vm_event_monitor_next_interrupt(struct vcpu *v);
>
> +int init_domain_vm_event(struct domain *d);
> +
>  #endif /* __VM_EVENT_H__ */
>
>  /*
> --
> 1.9.1

Tamas

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

Re: [Xen-devel] [PATCH v4 1/2] x86/monitor: add masking support for write_ctrlreg events

[Tamas K Lengyel]

On Wed, Jun 21, 2017 at 7:58 AM, Wei Liu <wei.l...@citrix.com> wrote:
> On Mon, Jun 19, 2017 at 03:24:38PM +0300, Petre Pircalabu wrote:
>> Add support for filtering out the write_ctrlreg monitor events if they
>> are generated only by changing certains bits.
>> A new parameter (bitmask) was added to the xc_monitor_write_ctrlreg
>> function in order to mask the event generation if the changed bits are
>> set.
>>
>> Signed-off-by: Petre Pircalabu <ppircal...@bitdefender.com>
>> Acked-by: Tamas K Lengyel <ta...@tklengyel.com>
>
> Coverity isn't happy with this patch.
>
> It seems to me there is indeed a risk to overrun the buffer (4 in size) 
> because
> the caller can specify index up to 31.

Indeed. We have a sanity check earlier in here that checks whether
index > 31 but it would make more sense to check it against the max
valid value of index to begin with (which at the moment is
VM_EVENT_X86_XCR0 = 3).

>
> ** CID 1412966:  Memory - corruptions  (OVERRUN)
> /xen/arch/x86/monitor.c: 162 in arch_monitor_domctl_event()
>
>
> 
> *** CID 1412966:  Memory - corruptions  (OVERRUN)
> /xen/arch/x86/monitor.c: 162 in arch_monitor_domctl_event()
> 156 ad->monitor.write_ctrlreg_onchangeonly |= ctrlreg_bitmask;
> 157 else
> 158 ad->monitor.write_ctrlreg_onchangeonly &= 
> ~ctrlreg_bitmask;
> 159
> 160 if ( requested_status )
> 161 {
>>>> CID 1412966:  Memory - corruptions  (OVERRUN)
>>>> Overrunning array "ad->monitor.write_ctrlreg_mask" of 4 8-byte 
>>>> elements at element index 31 (byte offset 248) using index 
>>>> "mop->u.mov_to_cr.index"
> (which evaluates to 31).
> 162 ad->monitor.write_ctrlreg_mask[mop->u.mov_to_cr.index] = 
> mop->u.mov_to_cr.bitmask;
> 163 ad->monitor.write_ctrlreg_enabled |= ctrlreg_bitmask;
> 164 }
> 165 else
> 166 {
> 167 ad->monitor.write_ctrlreg_mask[mop->u.mov_to_cr.index] = 
> 0;

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

Re: [Xen-devel] [PATCH v4 9/9] arm/mem_access: Walk the guest's pt in software

[Tamas K Lengyel]

On Tue, Jun 20, 2017 at 2:33 PM, Sergej Proskurin
<prosku...@sec.in.tum.de> wrote:
> In this commit, we make use of the gpt walk functionality introduced in
> the previous commits. If mem_access is active, hardware-based gva to ipa
> translation might fail, as gva_to_ipa uses the guest's translation
> tables, access to which might be restricted by the active VTTBR. To
> side-step potential translation errors in the function
> p2m_mem_access_check_and_get_page due to restricted memory (e.g. to the
> guest's page tables themselves), we walk the guest's page tables in
> software.
>
> Signed-off-by: Sergej Proskurin <prosku...@sec.in.tum.de>

Acked-by: Tamas K Lengyel <ta...@tklengyel.com>

> ---
> Cc: Razvan Cojocaru <rcojoc...@bitdefender.com>
> Cc: Tamas K Lengyel <ta...@tklengyel.com>
> Cc: Stefano Stabellini <sstabell...@kernel.org>
> Cc: Julien Grall <julien.gr...@arm.com>
> ---
> v2: Check the returned access rights after walking the guest's page tables in
> the function p2m_mem_access_check_and_get_page.
>
> v3: Adapt Function names and parameter.
>
> v4: Comment why we need to fail if the permission flags that are
> requested by the caller do not satisfy the mapped page.
>
> Cosmetic fix that simplifies the if-statement checking for the
> GV2M_WRITE permission.
> ---
>  xen/arch/arm/mem_access.c | 31 ++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/mem_access.c b/xen/arch/arm/mem_access.c
> index bcf49f5c15..9133ac8f03 100644
> --- a/xen/arch/arm/mem_access.c
> +++ b/xen/arch/arm/mem_access.c
> @@ -22,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  static int __p2m_get_mem_access(struct domain *d, gfn_t gfn,
>  xenmem_access_t *access)
> @@ -101,6 +102,7 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned 
> long flag,
>const struct vcpu *v)
>  {
>  long rc;
> +unsigned int perms;
>  paddr_t ipa;
>  gfn_t gfn;
>  mfn_t mfn;
> @@ -110,8 +112,35 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned 
> long flag,
>  struct p2m_domain *p2m = >domain->arch.p2m;
>
>  rc = gva_to_ipa(gva, , flag);
> +
> +/*
> + * In case mem_access is active, hardware-based gva_to_ipa translation
> + * might fail. Since gva_to_ipa uses the guest's translation tables, 
> access
> + * to which might be restricted by the active VTTBR, we perform a gva to
> + * ipa translation in software.
> + */
>  if ( rc < 0 )
> -goto err;
> +{
> +if ( guest_walk_tables(v, gva, , ) < 0 )
> +/*
> + * The software gva to ipa translation can still fail, e.g., if 
> the
> + * gva is not mapped.
> + */

If you end up sending another round of the series, I would prefer to
see this comment before the if statement (but I wouldn't hold up the
series over that).

> +goto err;
> +

Thanks,
Tamas

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

Re: [Xen-devel] [PATCH 2/2] memory: don't hand MFN info to translated guests

[Tamas K Lengyel]

> The method I found to work is getting the maximum_gpfn from the guest
> and then calling populate_physmap with ++max_gpfn. The only problem
> then is that I don't see a way to "unpopulate" the page from the
> domain and free the corresponding mfn while the domain is running. Is
> that currently possible to do?

Never mind, evidently XENMEM_remove_from_physmap seems to be the
answer, it just lacks a libxc wrapper so I didn't notice it.

Cheers,
Tamas

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

Re: [Xen-devel] [PATCH 2/2] memory: don't hand MFN info to translated guests

[Tamas K Lengyel]

On Mon, Jun 19, 2017 at 9:34 AM, Julien Grall <julien.gr...@arm.com> wrote:
>
>
> On 19/06/17 15:57, Tamas K Lengyel wrote:
>>
>> On Mon, Jun 19, 2017 at 8:52 AM, Julien Grall <julien.gr...@arm.com>
>> wrote:
>>>
>>>
>>>
>>> On 19/06/17 15:39, Tamas K Lengyel wrote:
>>>>
>>>>
>>>> On Mon, Jun 19, 2017 at 3:09 AM, Julien Grall <julien.gr...@arm.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>>
>>>>> On 19/06/17 09:15, Jan Beulich wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 18.06.17 at 21:19, <tamas.k.leng...@gmail.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Tue, Apr 4, 2017 at 1:04 PM, Andrew Cooper
>>>>>>> <andrew.coop...@citrix.com>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 04/04/17 14:14, Jan Beulich wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> We shouldn't hand MFN info back from increase-reservation for
>>>>>>>>> translated domains, just like we don't for populate-physmap and
>>>>>>>>> memory-exchange. For full symmetry also check for a NULL guest
>>>>>>>>> handle
>>>>>>>>> in populate_physmap() (but note this makes no sense in
>>>>>>>>> memory_exchange(), as there the array is also an input).
>>>>>>>>>
>>>>>>>>> Signed-off-by: Jan Beulich <jbeul...@suse.com>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Unfortunately I just had time to do testing with this change and I
>>>>>>> have to report that introduces a critical regression for my tools.
>>>>>>> With this change in-place performing increase_reservation on a target
>>>>>>> domain no longer reports the guest frame number for external tools,
>>>>>>> thus completely breaking advanced use-cases that require this
>>>>>>> information to be able to do altp2m gfn remapping. This is a critical
>>>>>>> step in being able to introduce shadow-pages that are used to hide
>>>>>>> breakpoints and other memory modifications from the guest.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> While I can see your point, I'm afraid that's not how the
>>>>>> interface was meant to be used. The mere fact that
>>>>>> populate-physmap and memory-exchange didn't return the
>>>>>> MFN(s) suggests to me that you already need to have a way
>>>>>> to deal with having to find out another way. Or are you
>>>>>> suggesting you rely on guests not using these interfaces?
>>>>>>
>>>>>> As to a solution, I could possibly see us relax the change to
>>>>>> return the MFN(s) when the current and subject domains differ,
>>>>>> or even check paging mode of the caller domain instead of the
>>>>>> subject one (which would mean PVH Dom0 still wouldn't get to
>>>>>> see them). But if we do, imo we should do this consistently for
>>>>>> all three operations, rather than just for increase-reservation.
>>>>>>
>>>>>>> If at all possible, I would like to request this change not to be
>>>>>>> part
>>>>>>> of the 4.9 release.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Hmm, it's been there for all of the RCs, so I'm not really happy
>>>>>> to consider the option of reverting at this point in time. But
>>>>>> Julien will have the final say anyway.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> I am a bit confuse with the descript

Re: [Xen-devel] [PATCH 2/2] memory: don't hand MFN info to translated guests

[Tamas K Lengyel]

On Mon, Jun 19, 2017 at 8:52 AM, Julien Grall <julien.gr...@arm.com> wrote:
>
>
> On 19/06/17 15:39, Tamas K Lengyel wrote:
>>
>> On Mon, Jun 19, 2017 at 3:09 AM, Julien Grall <julien.gr...@arm.com>
>> wrote:
>>>
>>> Hi,
>>>
>>>
>>> On 19/06/17 09:15, Jan Beulich wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 18.06.17 at 21:19, <tamas.k.leng...@gmail.com> wrote:
>>>>>
>>>>>
>>>>> On Tue, Apr 4, 2017 at 1:04 PM, Andrew Cooper
>>>>> <andrew.coop...@citrix.com>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> On 04/04/17 14:14, Jan Beulich wrote:
>>>>>>>
>>>>>>>
>>>>>>> We shouldn't hand MFN info back from increase-reservation for
>>>>>>> translated domains, just like we don't for populate-physmap and
>>>>>>> memory-exchange. For full symmetry also check for a NULL guest handle
>>>>>>> in populate_physmap() (but note this makes no sense in
>>>>>>> memory_exchange(), as there the array is also an input).
>>>>>>>
>>>>>>> Signed-off-by: Jan Beulich <jbeul...@suse.com>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com>
>>>>>
>>>>>
>>>>>
>>>>> Unfortunately I just had time to do testing with this change and I
>>>>> have to report that introduces a critical regression for my tools.
>>>>> With this change in-place performing increase_reservation on a target
>>>>> domain no longer reports the guest frame number for external tools,
>>>>> thus completely breaking advanced use-cases that require this
>>>>> information to be able to do altp2m gfn remapping. This is a critical
>>>>> step in being able to introduce shadow-pages that are used to hide
>>>>> breakpoints and other memory modifications from the guest.
>>>>
>>>>
>>>>
>>>> While I can see your point, I'm afraid that's not how the
>>>> interface was meant to be used. The mere fact that
>>>> populate-physmap and memory-exchange didn't return the
>>>> MFN(s) suggests to me that you already need to have a way
>>>> to deal with having to find out another way. Or are you
>>>> suggesting you rely on guests not using these interfaces?
>>>>
>>>> As to a solution, I could possibly see us relax the change to
>>>> return the MFN(s) when the current and subject domains differ,
>>>> or even check paging mode of the caller domain instead of the
>>>> subject one (which would mean PVH Dom0 still wouldn't get to
>>>> see them). But if we do, imo we should do this consistently for
>>>> all three operations, rather than just for increase-reservation.
>>>>
>>>>> If at all possible, I would like to request this change not to be part
>>>>> of the 4.9 release.
>>>>
>>>>
>>>>
>>>> Hmm, it's been there for all of the RCs, so I'm not really happy
>>>> to consider the option of reverting at this point in time. But
>>>> Julien will have the final say anyway.
>>>
>>>
>>>
>>> I am a bit confuse with the description of the problem. I understood
>>> "guest
>>> frame number" as GFN. But AFAICT, this hypercall was returning MFN even
>>> for
>>> HVM guests. So how this change is breaking altp2m remapping?
>>
>>
>> For HVM guests this hypercall returns a GFN that can subsequently be
>> populated into the guest physmap:
>>
>> xc_domain_increase_reservation_exact(xch, domid, 1, 0, 0, _gfn);
>> xc_domain_populate_physmap_exact(xch, domid, 1, 0, 0, _gfn);
>
>
> I am sorry, I can't see how this can return a GFN for the HVM. Looking at
> the implementation of increase_reservation in Xen:
>
> mfn = page_to_mfn(page);
> if ( unlikely(__copy_to_guest_offset(a->extent_list, i, , 1)) )
>   goto out;
>
> This is an MFN and not a GFN. Except the strict check before, the code has
> not change for a while.
>
> AFAICT, the purpose of increase_reservation is not to allocate a new GFN, it
> will just allocate the host memory for it. At least on ARM we have nothing
> to say "this GFN region is free". I would be surprised that such things
> exists on x86.
>

It returns memory that can be mapped into the guest physmap
subsequently. So I have been referring to it as a GFN that is not
mapped into the physmap - similar to the magic ring pages when they
are in use.

Tamas

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

Re: [Xen-devel] [PATCH 2/2] memory: don't hand MFN info to translated guests

[Tamas K Lengyel]

On Mon, Jun 19, 2017 at 8:54 AM, George Dunlap <george.dun...@citrix.com> wrote:
> On 19/06/17 15:48, Tamas K Lengyel wrote:
>> On Mon, Jun 19, 2017 at 3:11 AM, George Dunlap <george.dun...@citrix.com> 
>> wrote:
>>> On 19/06/17 09:15, Jan Beulich wrote:
>>>>>>> On 18.06.17 at 21:19, <tamas.k.leng...@gmail.com> wrote:
>>>>> On Tue, Apr 4, 2017 at 1:04 PM, Andrew Cooper <andrew.coop...@citrix.com>
>>>>> wrote:
>>>>>> On 04/04/17 14:14, Jan Beulich wrote:
>>>>>>> We shouldn't hand MFN info back from increase-reservation for
>>>>>>> translated domains, just like we don't for populate-physmap and
>>>>>>> memory-exchange. For full symmetry also check for a NULL guest handle
>>>>>>> in populate_physmap() (but note this makes no sense in
>>>>>>> memory_exchange(), as there the array is also an input).
>>>>>>>
>>>>>>> Signed-off-by: Jan Beulich <jbeul...@suse.com>
>>>>>>
>>>>>> Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com>
>>>>>
>>>>> Unfortunately I just had time to do testing with this change and I
>>>>> have to report that introduces a critical regression for my tools.
>>>>> With this change in-place performing increase_reservation on a target
>>>>> domain no longer reports the guest frame number for external tools,
>>>>> thus completely breaking advanced use-cases that require this
>>>>> information to be able to do altp2m gfn remapping. This is a critical
>>>>> step in being able to introduce shadow-pages that are used to hide
>>>>> breakpoints and other memory modifications from the guest.
>>>>
>>>> While I can see your point, I'm afraid that's not how the
>>>> interface was meant to be used.
>>>
>>> Well the first question to ask is, is that hypercall part of the stable
>>> interface?  If so, then the standard should be, "Don't break people who
>>> call it unless there is really no other way around it."  Sure, it was a
>>> mistake whoever introduced that, but if Tamas is building on a "stable"
>>> interface he should be able to rely on that interface being maintained,
>>> at least until we can find a suitable replacement.
>>>
>>>  -George
>>>
>>
>> Of course if a suitable replacement can be made that gets me the
>> information I need that would work too. At the moment I'm not aware of
>> any other hypercall I could use for this purpose.
>
> So actually -- it sounds like both Jan and I misunderstood the
> situation.  The header file clearly says:
>
>  * XENMEM_increase_reservation:
>  *   OUT: MFN (*not* GMFN) bases of extents that were allocated
>
> Are you saying that for HVM guests, that statement is false?
>

Well, it would certainly appear so as I  have been using it to add
memory to a guest and then map it into the guest physmap as a new gfn.
I've been using it like that since Xen 4.6 without any problems.

Tamas

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

Re: [Xen-devel] [PATCH v4 2/2] xen-access: write_ctrlreg_c4 test

[Tamas K Lengyel]

On Mon, Jun 19, 2017 at 6:24 AM, Petre Pircalabu
<ppircal...@bitdefender.com> wrote:
> Add test for write_ctrlreg event handling.
>
> Signed-off-by: Petre Pircalabu <ppircal...@bitdefender.com>

Acked-by: Tamas K Lengyel <ta...@tklengyel.com>

> ---
>  tools/tests/xen-access/xen-access.c | 53 
> -
>  1 file changed, 52 insertions(+), 1 deletion(-)
>
> diff --git a/tools/tests/xen-access/xen-access.c 
> b/tools/tests/xen-access/xen-access.c
> index 238011e..1e69e25 100644
> --- a/tools/tests/xen-access/xen-access.c
> +++ b/tools/tests/xen-access/xen-access.c
> @@ -57,6 +57,13 @@
>  #define X86_TRAP_DEBUG  1
>  #define X86_TRAP_INT3   3
>
> +/* From xen/include/asm-x86/x86-defns.h */
> +#define X86_CR4_PGE0x0080 /* enable global pages */
> +
> +#ifndef ARRAY_SIZE
> +#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
> +#endif
> +
>  typedef struct vm_event {
>  domid_t domain_id;
>  xenevtchn_handle *xce_handle;
> @@ -314,6 +321,24 @@ static void get_request(vm_event_t *vm_event, 
> vm_event_request_t *req)
>  }
>
>  /*
> + * X86 control register names
> + */
> +static const char* get_x86_ctrl_reg_name(uint32_t index)
> +{
> +static const char* names[] = {
> +[VM_EVENT_X86_CR0]  = "CR0",
> +[VM_EVENT_X86_CR3]  = "CR3",
> +[VM_EVENT_X86_CR4]  = "CR4",
> +[VM_EVENT_X86_XCR0] = "XCR0",
> +};
> +
> +if ( index >= ARRAY_SIZE(names) || names[index] == NULL )
> +return "";
> +
> +return names[index];
> +}
> +
> +/*
>   * Note that this function is not thread safe.
>   */
>  static void put_response(vm_event_t *vm_event, vm_event_response_t *rsp)
> @@ -337,7 +362,7 @@ void usage(char* progname)
>  {
>  fprintf(stderr, "Usage: %s [-m]  write|exec", progname);
>  #if defined(__i386__) || defined(__x86_64__)
> -fprintf(stderr, 
> "|breakpoint|altp2m_write|altp2m_exec|debug|cpuid|desc_access");
> +fprintf(stderr, 
> "|breakpoint|altp2m_write|altp2m_exec|debug|cpuid|desc_access|write_ctrlreg_cr4");
>  #elif defined(__arm__) || defined(__aarch64__)
>  fprintf(stderr, "|privcall");
>  #endif
> @@ -369,6 +394,7 @@ int main(int argc, char *argv[])
>  int debug = 0;
>  int cpuid = 0;
>  int desc_access = 0;
> +int write_ctrlreg_cr4 = 1;
>  uint16_t altp2m_view_id = 0;
>
>  char* progname = argv[0];
> @@ -439,6 +465,10 @@ int main(int argc, char *argv[])
>  {
>  desc_access = 1;
>  }
> +else if ( !strcmp(argv[0], "write_ctrlreg_cr4") )
> +{
> +write_ctrlreg_cr4 = 1;
> +}
>  #elif defined(__arm__) || defined(__aarch64__)
>  else if ( !strcmp(argv[0], "privcall") )
>  {
> @@ -596,6 +626,18 @@ int main(int argc, char *argv[])
>  }
>  }
>
> +if ( write_ctrlreg_cr4 )
> +{
> +/* Mask the CR4.PGE bit so no events will be generated for global 
> TLB flushes. */
> +rc = xc_monitor_write_ctrlreg(xch, domain_id, VM_EVENT_X86_CR4, 1, 1,
> +  X86_CR4_PGE, 1);
> +if ( rc < 0 )
> +{
> +ERROR("Error %d setting write control register trapping with 
> vm_event\n", rc);
> +goto exit;
> +}
> +}
> +
>  /* Wait for access */
>  for (;;)
>  {
> @@ -806,6 +848,15 @@ int main(int argc, char *argv[])
> req.u.desc_access.is_write);
>  rsp.flags |= VM_EVENT_FLAG_EMULATE;
>  break;
> +case VM_EVENT_REASON_WRITE_CTRLREG:
> +printf("Control register written: rip=%016"PRIx64", vcpu %d: 
> "
> +   "reg=%s, old_value=%016"PRIx64", 
> new_value=%016"PRIx64"\n",
> +   req.data.regs.x86.rip,
> +   req.vcpu_id,
> +   get_x86_ctrl_reg_name(req.u.write_ctrlreg.index),
> +   req.u.write_ctrlreg.old_value,
> +   req.u.write_ctrlreg.new_value);
> +break;
>  default:
>  fprintf(stderr, "UNKNOWN REASON CODE %d\n", req.reason);
>  }
> --
> 2.7.4

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

Re: [Xen-devel] [PATCH 2/2] memory: don't hand MFN info to translated guests

[Tamas K Lengyel]

On Mon, Jun 19, 2017 at 3:11 AM, George Dunlap  wrote:
> On 19/06/17 09:15, Jan Beulich wrote:
> On 18.06.17 at 21:19,  wrote:
>>> On Tue, Apr 4, 2017 at 1:04 PM, Andrew Cooper 
>>> wrote:
 On 04/04/17 14:14, Jan Beulich wrote:
> We shouldn't hand MFN info back from increase-reservation for
> translated domains, just like we don't for populate-physmap and
> memory-exchange. For full symmetry also check for a NULL guest handle
> in populate_physmap() (but note this makes no sense in
> memory_exchange(), as there the array is also an input).
>
> Signed-off-by: Jan Beulich 

 Reviewed-by: Andrew Cooper 
>>>
>>> Unfortunately I just had time to do testing with this change and I
>>> have to report that introduces a critical regression for my tools.
>>> With this change in-place performing increase_reservation on a target
>>> domain no longer reports the guest frame number for external tools,
>>> thus completely breaking advanced use-cases that require this
>>> information to be able to do altp2m gfn remapping. This is a critical
>>> step in being able to introduce shadow-pages that are used to hide
>>> breakpoints and other memory modifications from the guest.
>>
>> While I can see your point, I'm afraid that's not how the
>> interface was meant to be used.
>
> Well the first question to ask is, is that hypercall part of the stable
> interface?  If so, then the standard should be, "Don't break people who
> call it unless there is really no other way around it."  Sure, it was a
> mistake whoever introduced that, but if Tamas is building on a "stable"
> interface he should be able to rely on that interface being maintained,
> at least until we can find a suitable replacement.
>
>  -George
>

Of course if a suitable replacement can be made that gets me the
information I need that would work too. At the moment I'm not aware of
any other hypercall I could use for this purpose.

Tamas

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

Re: [Xen-devel] [PATCH 2/2] memory: don't hand MFN info to translated guests

[Tamas K Lengyel]

On Mon, Jun 19, 2017 at 3:09 AM, Julien Grall  wrote:
> Hi,
>
>
> On 19/06/17 09:15, Jan Beulich wrote:
>
> On 18.06.17 at 21:19,  wrote:
>>>
>>> On Tue, Apr 4, 2017 at 1:04 PM, Andrew Cooper 
>>> wrote:

 On 04/04/17 14:14, Jan Beulich wrote:
>
> We shouldn't hand MFN info back from increase-reservation for
> translated domains, just like we don't for populate-physmap and
> memory-exchange. For full symmetry also check for a NULL guest handle
> in populate_physmap() (but note this makes no sense in
> memory_exchange(), as there the array is also an input).
>
> Signed-off-by: Jan Beulich 


 Reviewed-by: Andrew Cooper 
>>>
>>>
>>> Unfortunately I just had time to do testing with this change and I
>>> have to report that introduces a critical regression for my tools.
>>> With this change in-place performing increase_reservation on a target
>>> domain no longer reports the guest frame number for external tools,
>>> thus completely breaking advanced use-cases that require this
>>> information to be able to do altp2m gfn remapping. This is a critical
>>> step in being able to introduce shadow-pages that are used to hide
>>> breakpoints and other memory modifications from the guest.
>>
>>
>> While I can see your point, I'm afraid that's not how the
>> interface was meant to be used. The mere fact that
>> populate-physmap and memory-exchange didn't return the
>> MFN(s) suggests to me that you already need to have a way
>> to deal with having to find out another way. Or are you
>> suggesting you rely on guests not using these interfaces?
>>
>> As to a solution, I could possibly see us relax the change to
>> return the MFN(s) when the current and subject domains differ,
>> or even check paging mode of the caller domain instead of the
>> subject one (which would mean PVH Dom0 still wouldn't get to
>> see them). But if we do, imo we should do this consistently for
>> all three operations, rather than just for increase-reservation.
>>
>>> If at all possible, I would like to request this change not to be part
>>> of the 4.9 release.
>>
>>
>> Hmm, it's been there for all of the RCs, so I'm not really happy
>> to consider the option of reverting at this point in time. But
>> Julien will have the final say anyway.
>
>
> I am a bit confuse with the description of the problem. I understood "guest
> frame number" as GFN. But AFAICT, this hypercall was returning MFN even for
> HVM guests. So how this change is breaking altp2m remapping?

For HVM guests this hypercall returns a GFN that can subsequently be
populated into the guest physmap:

xc_domain_increase_reservation_exact(xch, domid, 1, 0, 0, _gfn);
xc_domain_populate_physmap_exact(xch, domid, 1, 0, 0, _gfn);

...
Copy page contents from old_gfn to new_gfn and inject breakpoints,
make other memory modifications
...

xc_altp2m_change_gfn(xch, domid, altp2m_id, old_gfn, new_gfn);

Without being able to introduce a new gfn into the HVM guest's
physmap, we are unable to create a shadow page. It doesn't break
altp2m remapping itself, it breaks a per-requisite step in introducing
the page to remap to.

Tamas

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

Re: [Xen-devel] [PATCH 2/2] memory: don't hand MFN info to translated guests

[Tamas K Lengyel]

On Tue, Apr 4, 2017 at 1:04 PM, Andrew Cooper  wrote:
> On 04/04/17 14:14, Jan Beulich wrote:
>> We shouldn't hand MFN info back from increase-reservation for
>> translated domains, just like we don't for populate-physmap and
>> memory-exchange. For full symmetry also check for a NULL guest handle
>> in populate_physmap() (but note this makes no sense in
>> memory_exchange(), as there the array is also an input).
>>
>> Signed-off-by: Jan Beulich 
>
> Reviewed-by: Andrew Cooper 

Unfortunately I just had time to do testing with this change and I
have to report that introduces a critical regression for my tools.
With this change in-place performing increase_reservation on a target
domain no longer reports the guest frame number for external tools,
thus completely breaking advanced use-cases that require this
information to be able to do altp2m gfn remapping. This is a critical
step in being able to introduce shadow-pages that are used to hide
breakpoints and other memory modifications from the guest.

If at all possible, I would like to request this change not to be part
of the 4.9 release.

Thanks,
Tamas

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

Re: [Xen-devel] [PATCH v2 1/2] x86/monitor: add masking support for write_ctrlreg events

[Tamas K Lengyel]

On Fri, Jun 16, 2017 at 9:33 AM, Jan Beulich <jbeul...@suse.com> wrote:
>>>> On 16.06.17 at 17:28, <ta...@tklengyel.com> wrote:
>> On Fri, Jun 16, 2017 at 9:15 AM, Jan Beulich <jbeul...@suse.com> wrote:
>>>>>> On 16.06.17 at 16:26, <ta...@tklengyel.com> wrote:
>>>> On Tue, May 30, 2017 at 3:46 AM, Petre Pircalabu
>>>> <ppircal...@bitdefender.com> wrote:
>>>>> Add support for filtering out the write_ctrlreg monitor events if they
>>>>> are generated only by changing certains bits.
>>>>> A new parameter (bitmask) was added to the xc_monitor_write_ctrlreg
>>>>> function in order to mask the event generation if the changed bits are
>>>>> set.
>>>>>
>>>>> Signed-off-by: Petre Pircalabu <ppircal...@bitdefender.com>
>>>>
>>>> Acked-by: Tamas K Lengyel <ta...@tklengyel.com>
>>>
>>> Are you btw in agreement with ...
>>>
>>>>> @@ -1107,6 +1107,11 @@ struct xen_domctl_monitor_op {
>>>>>  uint8_t sync;
>>>>>  /* Send event only on a change of value */
>>>>>  uint8_t onchangeonly;
>>>>> +/*
>>>>> + * Send event only if the changed bit in the control register
>>>>> + * is not masked.
>>>>> + */
>>>>> +uint64_aligned_t bitmask;
>>>
>>> ... the 5-byte gap being introduced here, using of which won't
>>> be possible without bumping XEN_DOMCTL_INTERFACE_VERSION
>>> again? Generally we aim at making padding explicit and checking
>>> it to be zero on input / providing zero on output.
>>
>> Yes, making the padding explicit would be better (but I'm not the
>> maintainer of the domctl interface so my ack doesn't cover that).
>
> Well, I have been waiting for a monitor maintainer ack to perhaps
> give my own for the little bit of it where a REST maintainer one
> would be needed. Irrespective of file level maintainership I think
> the public interface parts still belong to their subsystems, and the
> REST maintainer ack should normally be a purely mechanical last
> step.

I can see the logic behind that. However, I would feel better if it
wasn't just a mechanical last step as for example I'm not as well
versed in the ABI side of things. Like in here, the padding bits have
completely went by me without me noticing.

Tamas

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

Re: [Xen-devel] [PATCH v2 1/2] x86/monitor: add masking support for write_ctrlreg events

[Tamas K Lengyel]

On Fri, Jun 16, 2017 at 9:15 AM, Jan Beulich <jbeul...@suse.com> wrote:
>>>> On 16.06.17 at 16:26, <ta...@tklengyel.com> wrote:
>> On Tue, May 30, 2017 at 3:46 AM, Petre Pircalabu
>> <ppircal...@bitdefender.com> wrote:
>>> Add support for filtering out the write_ctrlreg monitor events if they
>>> are generated only by changing certains bits.
>>> A new parameter (bitmask) was added to the xc_monitor_write_ctrlreg
>>> function in order to mask the event generation if the changed bits are
>>> set.
>>>
>>> Signed-off-by: Petre Pircalabu <ppircal...@bitdefender.com>
>>
>> Acked-by: Tamas K Lengyel <ta...@tklengyel.com>
>
> Are you btw in agreement with ...
>
>>> @@ -1107,6 +1107,11 @@ struct xen_domctl_monitor_op {
>>>  uint8_t sync;
>>>  /* Send event only on a change of value */
>>>  uint8_t onchangeonly;
>>> +/*
>>> + * Send event only if the changed bit in the control register
>>> + * is not masked.
>>> + */
>>> +uint64_aligned_t bitmask;
>
> ... the 5-byte gap being introduced here, using of which won't
> be possible without bumping XEN_DOMCTL_INTERFACE_VERSION
> again? Generally we aim at making padding explicit and checking
> it to be zero on input / providing zero on output.

Yes, making the padding explicit would be better (but I'm not the
maintainer of the domctl interface so my ack doesn't cover that).

Tamas

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

Re: [Xen-devel] [PATCH v2 2/2] xen-access: write_ctrlreg_c4 test

[Tamas K Lengyel]

On Fri, Jun 16, 2017 at 9:12 AM, Jan Beulich  wrote:
 On 16.06.17 at 16:32,  wrote:
>> On Tue, May 30, 2017 at 3:46 AM, Petre Pircalabu 
>>  wrote:
>>> @@ -314,6 +317,22 @@ static void get_request(vm_event_t *vm_event, 
>>> vm_event_request_t *req)
>>>  }
>>>
>>>  /*
>>> + * X86 control register names
>>> + */
>>> +static const char* get_x86_ctrl_reg_name(uint32_t index)
>>> +{
>>> +static const char* names[] = {
>>
>> I would prefer to see this being defined in the following form so that
>> it is clear where the indexes come from:
>>   [VM_EVENT_X86_CR0] = "CR0",
>>   ...
>>
>>> +"CR0",
>>> +"CR3",
>>> +"CR4",
>>> +"XCR0",
>>> +};
>>
>> And this check to be index > VM_EVENT_X86_XCR0
>
> Or perhaps even better >= ARRAY_SIZE()?

Yeap, even better.

Tamas

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

Re: [Xen-devel] [PATCH v2 2/2] xen-access: write_ctrlreg_c4 test

[Tamas K Lengyel]

On Tue, May 30, 2017 at 3:46 AM, Petre Pircalabu
 wrote:
> Add test for write_ctrlreg event handling.
>
> Signed-off-by: Petre Pircalabu 
> ---
>  tools/tests/xen-access/xen-access.c | 47 
> -
>  1 file changed, 46 insertions(+), 1 deletion(-)
>
> diff --git a/tools/tests/xen-access/xen-access.c 
> b/tools/tests/xen-access/xen-access.c
> index 238011e..29b60e8 100644
> --- a/tools/tests/xen-access/xen-access.c
> +++ b/tools/tests/xen-access/xen-access.c
> @@ -57,6 +57,9 @@
>  #define X86_TRAP_DEBUG  1
>  #define X86_TRAP_INT3   3
>
> +/* From xen/include/asm-x86/x86-defns.h */
> +#define X86_CR4_PGE0x0080 /* enable global pages */
> +
>  typedef struct vm_event {
>  domid_t domain_id;
>  xenevtchn_handle *xce_handle;
> @@ -314,6 +317,22 @@ static void get_request(vm_event_t *vm_event, 
> vm_event_request_t *req)
>  }
>
>  /*
> + * X86 control register names
> + */
> +static const char* get_x86_ctrl_reg_name(uint32_t index)
> +{
> +static const char* names[] = {

I would prefer to see this being defined in the following form so that
it is clear where the indexes come from:
  [VM_EVENT_X86_CR0] = "CR0",
  ...

> +"CR0",
> +"CR3",
> +"CR4",
> +"XCR0",
> +};

And this check to be index > VM_EVENT_X86_XCR0

> +return (index > 3) ? "" : names[index];
> +}
> +
> +
> +/*
>   * Note that this function is not thread safe.
>   */
>  static void put_response(vm_event_t *vm_event, vm_event_response_t *rsp)
> @@ -337,7 +356,7 @@ void usage(char* progname)
>  {
>  fprintf(stderr, "Usage: %s [-m]  write|exec", progname);
>  #if defined(__i386__) || defined(__x86_64__)
> -fprintf(stderr, 
> "|breakpoint|altp2m_write|altp2m_exec|debug|cpuid|desc_access");
> +fprintf(stderr, 
> "|breakpoint|altp2m_write|altp2m_exec|debug|cpuid|desc_access|write_ctrlreg_cr4");
>  #elif defined(__arm__) || defined(__aarch64__)
>  fprintf(stderr, "|privcall");
>  #endif
> @@ -369,6 +388,7 @@ int main(int argc, char *argv[])
>  int debug = 0;
>  int cpuid = 0;
>  int desc_access = 0;
> +int write_ctrlreg_cr4 = 1;
>  uint16_t altp2m_view_id = 0;
>
>  char* progname = argv[0];
> @@ -439,6 +459,10 @@ int main(int argc, char *argv[])
>  {
>  desc_access = 1;
>  }
> +else if ( !strcmp(argv[0], "write_ctrlreg_cr4") )
> +{
> +write_ctrlreg_cr4 = 1;
> +}
>  #elif defined(__arm__) || defined(__aarch64__)
>  else if ( !strcmp(argv[0], "privcall") )
>  {
> @@ -596,6 +620,18 @@ int main(int argc, char *argv[])
>  }
>  }
>
> +if ( write_ctrlreg_cr4 )
> +{
> +/* Mask the CR4.PGE bit so no events will be generated for global 
> TLB flushes. */
> +rc = xc_monitor_write_ctrlreg(xch, domain_id, VM_EVENT_X86_CR4, 1, 1,
> +  X86_CR4_PGE, 1);
> +if ( rc < 0 )
> +{
> +ERROR("Error %d setting write control register trapping with 
> vm_event\n", rc);
> +goto exit;
> +}
> +}
> +
>  /* Wait for access */
>  for (;;)
>  {
> @@ -806,6 +842,15 @@ int main(int argc, char *argv[])
> req.u.desc_access.is_write);
>  rsp.flags |= VM_EVENT_FLAG_EMULATE;
>  break;
> +case VM_EVENT_REASON_WRITE_CTRLREG:
> +printf("Control register written: rip=%016"PRIx64", vcpu %d: 
> "
> +   "reg=%s, old_value=%016"PRIx64", 
> new_value=%016"PRIx64"\n",
> +   req.data.regs.x86.rip,
> +   req.vcpu_id,
> +   get_x86_ctrl_reg_name(req.u.write_ctrlreg.index),
> +   req.u.write_ctrlreg.old_value,
> +   req.u.write_ctrlreg.new_value);
> +break;
>  default:
>  fprintf(stderr, "UNKNOWN REASON CODE %d\n", req.reason);
>  }
> --
> 2.7.4

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

Re: [Xen-devel] [PATCH v2 1/2] x86/monitor: add masking support for write_ctrlreg events

[Tamas K Lengyel]

On Tue, May 30, 2017 at 3:46 AM, Petre Pircalabu
<ppircal...@bitdefender.com> wrote:
> Add support for filtering out the write_ctrlreg monitor events if they
> are generated only by changing certains bits.
> A new parameter (bitmask) was added to the xc_monitor_write_ctrlreg
> function in order to mask the event generation if the changed bits are
> set.
>
> Signed-off-by: Petre Pircalabu <ppircal...@bitdefender.com>

Acked-by: Tamas K Lengyel <ta...@tklengyel.com>

> ---
>  tools/libxc/include/xenctrl.h | 2 +-
>  tools/libxc/xc_monitor.c  | 3 ++-
>  xen/arch/x86/hvm/monitor.c| 3 ++-
>  xen/arch/x86/monitor.c| 6 ++
>  xen/include/asm-x86/domain.h  | 1 +
>  xen/include/public/domctl.h   | 7 ++-
>  6 files changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 1629f41..8c26cb4 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1999,7 +1999,7 @@ int xc_monitor_get_capabilities(xc_interface *xch, 
> domid_t domain_id,
>  uint32_t *capabilities);
>  int xc_monitor_write_ctrlreg(xc_interface *xch, domid_t domain_id,
>   uint16_t index, bool enable, bool sync,
> - bool onchangeonly);
> + uint64_t bitmask, bool onchangeonly);
>  /*
>   * A list of MSR indices can usually be found in 
> /usr/include/asm/msr-index.h.
>   * Please consult the Intel/AMD manuals for more information on
> diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
> index f99b6e3..70648d7 100644
> --- a/tools/libxc/xc_monitor.c
> +++ b/tools/libxc/xc_monitor.c
> @@ -70,7 +70,7 @@ int xc_monitor_get_capabilities(xc_interface *xch, domid_t 
> domain_id,
>
>  int xc_monitor_write_ctrlreg(xc_interface *xch, domid_t domain_id,
>   uint16_t index, bool enable, bool sync,
> - bool onchangeonly)
> + uint64_t bitmask, bool onchangeonly)
>  {
>  DECLARE_DOMCTL;
>
> @@ -82,6 +82,7 @@ int xc_monitor_write_ctrlreg(xc_interface *xch, domid_t 
> domain_id,
>  domctl.u.monitor_op.u.mov_to_cr.index = index;
>  domctl.u.monitor_op.u.mov_to_cr.sync = sync;
>  domctl.u.monitor_op.u.mov_to_cr.onchangeonly = onchangeonly;
> +domctl.u.monitor_op.u.mov_to_cr.bitmask = bitmask;
>
>  return do_domctl(xch, );
>  }
> diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c
> index bde5fd0..a7ccfc4 100644
> --- a/xen/arch/x86/hvm/monitor.c
> +++ b/xen/arch/x86/hvm/monitor.c
> @@ -38,7 +38,8 @@ bool_t hvm_monitor_cr(unsigned int index, unsigned long 
> value, unsigned long old
>
>  if ( (ad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask) &&
>   (!(ad->monitor.write_ctrlreg_onchangeonly & ctrlreg_bitmask) ||
> -  value != old) )
> +  value != old) &&
> + (!((value ^ old) & ad->monitor.write_ctrlreg_mask[index])) )
>  {
>  bool_t sync = !!(ad->monitor.write_ctrlreg_sync & ctrlreg_bitmask);
>
> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
> index 449c64c..d02d2ea 100644
> --- a/xen/arch/x86/monitor.c
> +++ b/xen/arch/x86/monitor.c
> @@ -155,9 +155,15 @@ int arch_monitor_domctl_event(struct domain *d,
>  ad->monitor.write_ctrlreg_onchangeonly &= ~ctrlreg_bitmask;
>
>  if ( requested_status )
> +{
> +ad->monitor.write_ctrlreg_mask[mop->u.mov_to_cr.index] = 
> mop->u.mov_to_cr.bitmask;
>  ad->monitor.write_ctrlreg_enabled |= ctrlreg_bitmask;
> +}
>  else
> +{
> +ad->monitor.write_ctrlreg_mask[mop->u.mov_to_cr.index] = 0;
>  ad->monitor.write_ctrlreg_enabled &= ~ctrlreg_bitmask;
> +}
>
>  if ( VM_EVENT_X86_CR3 == mop->u.mov_to_cr.index )
>  {
> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index 924caac..27d80ee 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -406,6 +406,7 @@ struct arch_domain
>  unsigned int cpuid_enabled   : 1;
>  unsigned int descriptor_access_enabled   : 1;
>  struct monitor_msr_bitmap *msr_bitmap;
> +uint64_t write_ctrlreg_mask[4];
>  } monitor;
>
>  /* Mem_access emulation control */
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index e6cf211..652fb17 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
&

Re: [Xen-devel] questions on mem_sharing_op*'s and tools/tests/mem-sharing/memshrtool

[Tamas K Lengyel]

On Thu, Jun 15, 2017 at 10:00 PM, Zhongze Liu  wrote:
> 2017-06-16 11:50 GMT+08:00 Zhongze Liu :
>> Hi there,
>>
>> I was experimenting with the mem_sharing_op and I found a handy tool:
>> tools/tests/mem-sharing/memshrtool
>> I set up two bare metal x86_64 VMS running some simple code in 16-bit
>> real mode and I tried to share the first physical page among them. so
>>
>
> And both of them are hvms and have a memory of 32M.
>
>>
>> I use:
>>
>> "./memshrtool enable src_dom "
>> "./memshrtool enable dst_dom"
>> "./memshrtool nominate src_dom 0"
>>
>> And it failed with an "error executing xc_memshr_nominate_gfn(xch,
>> domid, gfn, ): Argument list too long "
>>
>> But when I changed the gfn from 0 to 1000, it succeeds. Is there any
>> restrition on the gfns to be shared?
>>
>
> and the minimum gfn for the call to succeed is 256.

Memory sharing only works on certain types of guest pages (see
http://xenbits.xen.org/hg/staging/xen-unstable.hg/file/dec449c24a0c/xen/include/asm-x86/p2m.h#l138).
Having guest pages which fail to nominate is thus normal. I would
guess that the first 256 pages are mapped either as mmio or simply as
a hole. Maybe someone with a bit more knowledge about HVM domain
construction can chime in why that is so.

Tamas

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

Re: [Xen-devel] [PATCH 2/2] x86/altp2m: Add a hvmop for setting the suppress #VE bit

[Tamas K Lengyel]

On Fri, Jun 9, 2017 at 10:51 AM, Adrian Pop  wrote:
> Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a
> privileged domain to change the value of the #VE suppress bit for a
> page.
>
> Add a libxc wrapper for invoking this hvmop.
>
> Signed-off-by: Adrian Pop 
> ---
>  tools/libxc/include/xenctrl.h   |  2 ++
>  tools/libxc/xc_altp2m.c | 24 +++
>  xen/arch/x86/hvm/hvm.c  | 14 +++
>  xen/arch/x86/mm/mem_access.c| 52 
> +
>  xen/include/public/hvm/hvm_op.h | 15 
>  xen/include/xen/mem_access.h|  3 +++
>  6 files changed, 110 insertions(+)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 1629f412dd..f6ba8635bf 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1926,6 +1926,8 @@ int xc_altp2m_destroy_view(xc_interface *handle, 
> domid_t domid,
>  /* Switch all vCPUs of the domain to the specified altp2m view */
>  int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
>   uint16_t view_id);
> +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid,
> +  uint16_t view_id, xen_pfn_t gfn, bool sve);
>  int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
>   uint16_t view_id, xen_pfn_t gfn,
>   xenmem_access_t access);
> diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
> index 0639632477..4710133918 100644
> --- a/tools/libxc/xc_altp2m.c
> +++ b/tools/libxc/xc_altp2m.c
> @@ -163,6 +163,30 @@ int xc_altp2m_switch_to_view(xc_interface *handle, 
> domid_t domid,
>  return rc;
>  }
>
> +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid,
> +  uint16_t view_id, xen_pfn_t gfn, bool sve)
> +{
> +int rc;
> +DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
> +
> +arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg));
> +if ( arg == NULL )
> +return -1;
> +
> +arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
> +arg->cmd = HVMOP_altp2m_set_suppress_ve;
> +arg->domain = domid;
> +arg->u.set_suppress_ve.view = view_id;
> +arg->u.set_suppress_ve.gfn = gfn;
> +arg->u.set_suppress_ve.suppress_ve = sve;
> +
> +rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
> + HYPERCALL_BUFFER_AS_ARG(arg));
> +
> +xc_hypercall_buffer_free(handle, arg);
> +return rc;
> +}
> +
>  int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
>   uint16_t view_id, xen_pfn_t gfn,
>   xenmem_access_t access)
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 70ddc81d44..dd8e205551 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4358,6 +4358,7 @@ static int do_altp2m_op(
>  case HVMOP_altp2m_destroy_p2m:
>  case HVMOP_altp2m_switch_p2m:
>  case HVMOP_altp2m_set_mem_access:
> +case HVMOP_altp2m_set_suppress_ve:
>  case HVMOP_altp2m_change_gfn:
>  break;
>  default:
> @@ -4475,6 +4476,19 @@ static int do_altp2m_op(
>  a.u.set_mem_access.view);
>  break;
>
> +case HVMOP_altp2m_set_suppress_ve:
> +if ( a.u.set_suppress_ve.pad1 || a.u.set_suppress_ve.pad2 )
> +rc = -EINVAL;
> +else
> +{
> +gfn_t gfn = _gfn(a.u.set_mem_access.gfn);
> +unsigned int altp2m_idx = a.u.set_mem_access.view;
> +bool suppress_ve = a.u.set_suppress_ve.suppress_ve;
> +
> +rc = p2m_set_suppress_ve(d, gfn, suppress_ve, altp2m_idx);
> +}
> +break;
> +
>  case HVMOP_altp2m_change_gfn:
>  if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 )
>  rc = -EINVAL;
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index d0b0767855..8c39db13e3 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -466,6 +466,58 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, 
> xenmem_access_t *access)
>  }
>
>  /*
> + * Set/clear the #VE suppress bit for a page.  Only available on VMX.
> + */
> +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
> +unsigned int altp2m_idx)
> +{
> +struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
> +struct p2m_domain *ap2m = NULL;
> +struct p2m_domain *p2m;
> +mfn_t mfn;
> +p2m_access_t a;
> +p2m_type_t t;
> +int rc;
> +
> +if ( !cpu_has_vmx_virt_exceptions )
> +return -EOPNOTSUPP;
> +
> +/* This subop should only be used from a privileged domain. */
> +if ( !current->domain->is_privileged )
> +return -EINVAL;

This check looks wrong to me. If this subop should only be used by an

Re: [Xen-devel] [PATCH 1/2] x86/mm: Change default value for suppress #VE in set_mem_access()

[Tamas K Lengyel]

On Fri, Jun 9, 2017 at 10:51 AM, Adrian Pop  wrote:
> From: Vlad Ioan Topan 
>
> The default value for the "suppress #VE" bit set by set_mem_access()
> currently depends on whether the call is made from the same domain (the
> bit is set when called from another domain and cleared if called from
> the same domain). This patch changes that behavior to inherit the old
> suppress #VE bit value if it is already set and to set it to 1
> otherwise, which is safer and more reliable.

Could you elaborate on why do you think it is safer and more reliable
to switch the behavior? I believe the original idea was that the
domain should only be allowed to clear an SVE bit set by an external
tool. With this change it will allow the guest to request VE for any
page the external tool hasn't itself reserved specifically.

Tamas

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

Re: [Xen-devel] [PATCH 15/24] xen/arm: Use the newly introduced MFN <-> MADDR and GFN <-> MADDR helpers

[Tamas K Lengyel]

On Tue, Jun 13, 2017 at 10:13 AM, Julien Grall <julien.gr...@arm.com> wrote:
> Replace the following constructions:
> - _gfn(paddr_to_pfn(...))   => gaddr_to_gfn(...)
> - _mfn(paddr_to_pfn(...))   => maddr_to_mfn(...)
> - pfn_to_paddr(mfn_x(...))  => mfn_to_maddr(...)
> - pfn_to_paddr(gfn_x(...))  => gfn_to_gaddr(...)
> - _mfn(... >> PAGE_SHIFT)   => maddr_to_mfn(...)
>
> Signed-off-by: Julien Grall <julien.gr...@arm.com>
> Cc: Razvan Cojocaru <rcojoc...@bitdefender.com>
> Cc: Tamas K Lengyel <ta...@tklengyel.com>

Cool, this makes things a lot more readable!

For the mem_access bits:
Acked-by: Tamas K Lengyel <ta...@tklengyel.com>

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

Re: [Xen-devel] [RFC PATCH v3 10/10] arm/mem_access: Walk the guest's pt in software

[Tamas K Lengyel]

On Thu, Jun 15, 2017 at 5:05 AM, Sergej Proskurin
<prosku...@sec.in.tum.de> wrote:
> In this commit, we make use of the gpt walk functionality introduced in
> the previous commits. If mem_access is active, hardware-based gva to ipa
> translation might fail, as gva_to_ipa uses the guest's translation
> tables, access to which might be restricted by the active VTTBR. To
> side-step potential translation errors in the function
> p2m_mem_access_check_and_get_page due to restricted memory (e.g. to the
> guest's page tables themselves), we walk the guest's page tables in
> software.
>
> Signed-off-by: Sergej Proskurin <prosku...@sec.in.tum.de>
> ---
> Cc: Razvan Cojocaru <rcojoc...@bitdefender.com>
> Cc: Tamas K Lengyel <ta...@tklengyel.com>
> Cc: Stefano Stabellini <sstabell...@kernel.org>
> Cc: Julien Grall <julien.gr...@arm.com>
> ---
> v2: Check the returned access rights after walking the guest's page tables in
> the function p2m_mem_access_check_and_get_page.
>
> v3: Adapt Function names and parameter.
> ---
>  xen/arch/arm/mem_access.c | 21 -
>  1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/mem_access.c b/xen/arch/arm/mem_access.c
> index 04b1506b00..acb5539bb6 100644
> --- a/xen/arch/arm/mem_access.c
> +++ b/xen/arch/arm/mem_access.c
> @@ -22,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  static int __p2m_get_mem_access(struct domain *d, gfn_t gfn,
>  xenmem_access_t *access)
> @@ -101,6 +102,7 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned 
> long flag,
>const struct vcpu *v)
>  {
>  long rc;
> +unsigned int perms;
>  paddr_t ipa;
>  gfn_t gfn;
>  mfn_t mfn;
> @@ -110,8 +112,25 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned 
> long flag,
>  struct p2m_domain *p2m = >domain->arch.p2m;
>
>  rc = gva_to_ipa(gva, , flag);
> +
> +/*
> + * In case mem_access is active, hardware-based gva_to_ipa translation
> + * might fail. Since gva_to_ipa uses the guest's translation tables, 
> access
> + * to which might be restricted by the active VTTBR, we perform a gva to
> + * ipa translation in software.
> + */
>  if ( rc < 0 )
> -goto err;
> +{
> +if ( guest_walk_tables(v, gva, , ) < 0 )
> +/*
> + * The software gva to ipa translation can still fail, e.g., if 
> the
> + * gva is not mapped.
> + */
> +goto err;
> +
> +if ( ((flag & GV2M_WRITE) == GV2M_WRITE) && !(perms & GV2M_WRITE) )

Wouldn't it be enough to do (flag & GV2M_WRITE) without the following
comparison? Also, a comment explaining why this is an error-condition
would be nice.

> +goto err;
> +}
>
>  gfn = _gfn(paddr_to_pfn(ipa));
>
> --
> 2.12.2

Thanks,
Tamas

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

Re: [Xen-devel] [PATCH 2/3] x86/altp2m: Add a hvmop for setting the suppress #VE bit

[Tamas K Lengyel]

On Thu, May 18, 2017 at 9:07 AM, Adrian Pop  wrote:
> Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a
> domain to change the value of the #VE suppress bit for a page.
>
> Signed-off-by: Adrian Pop 
> ---
>  xen/arch/x86/hvm/hvm.c  | 14 
>  xen/arch/x86/mm/mem_access.c| 48 
> +
>  xen/include/public/hvm/hvm_op.h | 15 +
>  xen/include/xen/mem_access.h|  3 +++
>  4 files changed, 80 insertions(+)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 2e76c2345b..eb01527c5b 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4356,6 +4356,7 @@ static int do_altp2m_op(
>  case HVMOP_altp2m_destroy_p2m:
>  case HVMOP_altp2m_switch_p2m:
>  case HVMOP_altp2m_set_mem_access:
> +case HVMOP_altp2m_set_suppress_ve:
>  case HVMOP_altp2m_change_gfn:
>  break;
>  default:
> @@ -4472,6 +4473,19 @@ static int do_altp2m_op(
>  a.u.set_mem_access.view);
>  break;
>
> +case HVMOP_altp2m_set_suppress_ve:
> +if ( a.u.set_suppress_ve.pad1 || a.u.set_suppress_ve.pad2 )
> +rc = -EINVAL;
> +else
> +{
> +gfn_t gfn = _gfn(a.u.set_mem_access.gfn);
> +unsigned int altp2m_idx = a.u.set_mem_access.view;
> +uint8_t suppress_ve = a.u.set_suppress_ve.suppress_ve;
> +
> +rc = p2m_set_suppress_ve(d, gfn, suppress_ve, altp2m_idx);
> +}
> +break;
> +
>  case HVMOP_altp2m_change_gfn:
>  if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 )
>  rc = -EINVAL;
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index d0b0767855..b9e611d3db 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -466,6 +466,54 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, 
> xenmem_access_t *access)
>  }
>
>  /*
> + * Set/clear the #VE suppress bit for a page.  Only available on VMX.
> + */
> +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, uint8_t suppress_ve,
> +unsigned int altp2m_idx)
> +{
> +struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
> +struct p2m_domain *ap2m = NULL;
> +struct p2m_domain *p2m = NULL;
> +mfn_t mfn;
> +p2m_access_t a;
> +p2m_type_t t;
> +unsigned long gfn_l;
> +int rc = 0;
> +
> +if ( !cpu_has_vmx )
> +return -EOPNOTSUPP;
> +
> +if ( altp2m_idx > 0 )
> +{
> +if ( altp2m_idx >= MAX_ALTP2M ||
> +d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
> +return -EINVAL;
> +
> +p2m = ap2m = d->arch.altp2m_p2m[altp2m_idx];
> +}
> +else
> +{
> +p2m = host_p2m;
> +}


IMHO there should be some further checks here to verify that the
domain has issued HVMOP_altp2m_vcpu_enable_notify before and that it
was allowed (ie. this hypercall should not be able to enable the
suppress_bit if there is no veinfo_gfn). That said, is there anything
that would prevent a malicious application issuing rouge altp2m HVMOPs
from messing with this if it is activated (which I guess stands for
the rest of the altp2m ops too)?

> +
> +p2m_lock(host_p2m);
> +if ( ap2m )
> +p2m_lock(ap2m);
> +
> +gfn_l = gfn_x(gfn);
> +mfn = p2m->get_entry(p2m, gfn_l, , , 0, NULL, NULL);
> +if ( !mfn_valid(mfn) )
> +return -ESRCH;
> +rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a,
> +suppress_ve);
> +if ( ap2m )
> +p2m_unlock(ap2m);
> +p2m_unlock(host_p2m);
> +
> +return rc;
> +}
> +
> +/*
>   * Local variables:
>   * mode: C
>   * c-file-style: "BSD"
> diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
> index bc00ef0e65..9736092f58 100644
> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -231,6 +231,18 @@ struct xen_hvm_altp2m_set_mem_access {
>  typedef struct xen_hvm_altp2m_set_mem_access xen_hvm_altp2m_set_mem_access_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t);
>
> +struct xen_hvm_altp2m_set_suppress_ve {
> +/* view */
> +uint16_t view;
> +uint8_t suppress_ve;
> +uint8_t pad1;
> +uint32_t pad2;
> +/* gfn */
> +uint64_t gfn;
> +};
> +typedef struct xen_hvm_altp2m_set_suppress_ve 
> xen_hvm_altp2m_set_suppress_ve_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_suppress_ve_t);
> +
>  struct xen_hvm_altp2m_change_gfn {
>  /* view */
>  uint16_t view;
> @@ -262,6 +274,8 @@ struct xen_hvm_altp2m_op {
>  #define HVMOP_altp2m_set_mem_access   7
>  /* Change a p2m entry to have a different gfn->mfn mapping */
>  #define HVMOP_altp2m_change_gfn   8
> +/* Set the "Suppress #VE" bit on a page */
> +#define HVMOP_altp2m_set_suppress_ve  9
>  domid_t domain;
>  uint16_t pad1;
> 

Re: [Xen-devel] [RFC PATCH 4/4] arm/mem_access: Add software guest-page-table walk

[Tamas K Lengyel]

On Tue, May 9, 2017 at 10:22 AM, Julien Grall <julien.gr...@arm.com> wrote:
>
>
> On 09/05/17 17:04, Tamas K Lengyel wrote:
>>
>> On Tue, May 9, 2017 at 2:09 AM, Julien Grall <julien.gr...@arm.com> wrote:
>>>
>>>
>>>
>>> On 05/09/2017 08:17 AM, Sergej Proskurin wrote:
>>>>
>>>>
>>>> Hi,
>>>>
>>>>>> What you currently do is try gva_to_ipa and if it does not work >> you
>>>>>> will call p2m_gva_to_ipa. This sounds a bit pointless to me and
>>>>>> waste of time if the underlying memory of stage-1 page table is >>
>>>>
>>>>
>>>> protected. > > But we don't know that the stage-1 page table is
>>>> protected until the > hardware based lookup fails. I can turn your
>>>> argument around and say > doing the software based lookup is pointless
>>>> and a waste of time > when the stage-1 table is not protected. Which is
>>>> by the way what I > would expect to see in most cases.
>>>>
>>>> I agree with Tamas: I also believe that in most cases the stage-1
>>>> translation table won't be protected. So, in my opinion, falling back to
>>>> software (which will be presumablly a rare operation) is the better
>>>> approach.
>>>
>>>
>>>
>>> Well, you both consider that it is better to do the fallback by assuming
>>> the
>>> TLBs will always cache the intermediate translations (e.g VA -> IPA) and
>>> not
>>> only the full S1 -> S2 translation (e.g VA -> PA).
>>
>>
>> No, I was just pointing out that if the TLB has it cached it is
>> guaranteed to be faster. Whether that is the case is probably usecase
>> dependent. But even if the TLB is not loaded, I would assume the
>> hardware lookup is at least as fast as the software based one, but I
>> would bet it is faster. Without number this is just theoretical but I
>> would be very surprised if the hardware lookup would ever be slower
>> then the software based one... And since this is a corner-case that
>> most application will never encounter, forcing them all to use a
>> slower approach doesn't sound right.
>
>
> What you miss in my point is TLB can be designed to never cache intermediate
> translation. It is not even about whether the TLB are loaded or not.
>
> I am struggling to understand why this would be a corner case as it would be
> valid to protect the page table to inspect what the guest is doing...

It is valid, yes, but nothing requires the user to block read access
to that specific page. Most applications only monitor a handful of
pages at a time. Applications may not even use read-access
restrictions at all (doing only write or execute tracing). If we do
software based lookups all the time we penalize all of those
applications when it is really only a very small subset of cases where
this will be needed.

>
> I am not saying translation would be slower in hardware than in software. I
> am just saying that maybe it would not be a huge performance hit to always
> do software rather than fallback and having a worse one time to time (or
> often?).

It probably wouldn't be a big hit, but why take any hit if not necessary?

Tamas

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

Re: [Xen-devel] [RFC PATCH 4/4] arm/mem_access: Add software guest-page-table walk

[Tamas K Lengyel]

On Tue, May 9, 2017 at 2:09 AM, Julien Grall  wrote:
>
>
> On 05/09/2017 08:17 AM, Sergej Proskurin wrote:
>>
>> Hi,
>>
 What you currently do is try gva_to_ipa and if it does not work >> you
 will call p2m_gva_to_ipa. This sounds a bit pointless to me and
 waste of time if the underlying memory of stage-1 page table is >>
>>
>> protected. > > But we don't know that the stage-1 page table is
>> protected until the > hardware based lookup fails. I can turn your
>> argument around and say > doing the software based lookup is pointless
>> and a waste of time > when the stage-1 table is not protected. Which is
>> by the way what I > would expect to see in most cases.
>>
>> I agree with Tamas: I also believe that in most cases the stage-1
>> translation table won't be protected. So, in my opinion, falling back to
>> software (which will be presumablly a rare operation) is the better
>> approach.
>
>
> Well, you both consider that it is better to do the fallback by assuming the
> TLBs will always cache the intermediate translations (e.g VA -> IPA) and not
> only the full S1 -> S2 translation (e.g VA -> PA).

No, I was just pointing out that if the TLB has it cached it is
guaranteed to be faster. Whether that is the case is probably usecase
dependent. But even if the TLB is not loaded, I would assume the
hardware lookup is at least as fast as the software based one, but I
would bet it is faster. Without number this is just theoretical but I
would be very surprised if the hardware lookup would ever be slower
then the software based one... And since this is a corner-case that
most application will never encounter, forcing them all to use a
slower approach doesn't sound right.

Tamas

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