On 18.05.2024 13:02, Petr Beneš wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -685,6 +685,18 @@ int arch_sanitise_domain_config(struct
> xen_domctl_createdomain *config)
> return -EINVAL;
> }
>
> + if ( config->nr_altp2m && !hvm_altp2m_supported() )
> + {
> + dprintk(XENLOG_INFO, "altp2m requested but not available\n");
> + return -EINVAL;
> + }
> +
> + if ( config->nr_altp2m > MAX_EPTP )
The compared entities don't really fit together. I think we want a new
MAX_NR_ALTP2M, which - for the time being - could simply be
#define MAX_NR_ALTP2M MAX_EPTP
in the header. That would then be a suitable replacement for the
min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) that you're adjusting
elsewhere. Which however raises the question whether in EPT-specific
code the min() wouldn't better survive, as min(d->nr_altp2m, MAX_EPTP).
> @@ -5228,7 +5234,7 @@ void hvm_fast_singlestep(struct vcpu *v, uint16_t
> p2midx)
> if ( !hvm_is_singlestep_supported() )
> return;
>
> - if ( p2midx >= MAX_ALTP2M )
> + if ( p2midx >= v->domain->nr_altp2m )
> return;
You don't introduce a new local variable here. I'd like to ask that you also
don't ...
> @@ -403,12 +403,12 @@ long p2m_set_mem_access_multi(struct domain *d,
> /* altp2m view 0 is treated as the hostp2m */
> if ( altp2m_idx )
> {
> - if ( altp2m_idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
> - d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] ==
> - mfn_x(INVALID_MFN) )
> + if ( altp2m_idx >= d->nr_altp2m ||
> + d->arch.altp2m_eptp[array_index_nospec(altp2m_idx,
> d->nr_altp2m)]
> + == mfn_x(INVALID_MFN) )
Please don't break previously correct style: Binary operators (here: == )
belong onto the end of the earlier line. That'll render the line too long
again, but you want to deal with that e.g. thus:
d->arch.altp2m_eptp[array_index_nospec(altp2m_idx,
d->nr_altp2m)] ==
mfn_x(INVALID_MFN) )
Jan
