Re: [RFC PATCH 1/4] xen/arm: justify or initialize conditionally uninitialized variables

[Julien Grall <julien@xxxxxxx>]

Hi,

The e-mail is getting quite long. Can you trim the unnecessary bits when 
replying?
On 20/07/2023 15:23, Nicola Vetrini wrote:
> > > > The problem is that _t may be uninitialized, hence assigning its 
> > > > address to t could be problematic.
> > > But the value is set right after. IOW, there is no read between. So 
> > > how is this prob
> > > > Another way to address this is to initialize _t to a bad value and 
> > > > use this variable in the body, then assign to t based on the value 
> > > > just before returning.
> > > IHMO, neither solution are ideal. I think we should investigate 
> > > whether Eclair can be improved.
> > > [...]
> > I'll see what can be done about it, I'll reply when I have an answer.
> What about this:
>
> -    p2m_type_t _t;
> +    p2m_type_t _t = p2m_invalid;
> [...]
>       t = t ?: &_t;
> -    *t = p2m_invalid;
> +    *t = _t;
The resulting code is still quite confusing. I am still not quite sure 
why ECLAIR can't understand the construct. Apologies if this was already 
said, but this thread is getting quite long with many different issues. 
So it is a bit difficult to navigate (which is why I suggested to split 
and have a commit message explaining the rationale for each).
Anyway, if we can't improve Eclair, then my preference would be the 
following:
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index de32a2d638ba..05d65db01b0c 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -496,16 +496,13 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
     lpae_t entry, *table;
     int rc;
     mfn_t mfn = INVALID_MFN;
-    p2m_type_t _t;
     DECLARE_OFFSETS(offsets, addr);

     ASSERT(p2m_is_locked(p2m));
     BUILD_BUG_ON(THIRD_MASK != PAGE_MASK);

-    /* Allow t to be NULL */
-    t = t ?: &_t;
-
-    *t = p2m_invalid;
+    if ( t )
+        *t = p2m_invalid;

     if ( valid )
         *valid = false;
@@ -549,7 +546,8 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,

     if ( p2m_is_valid(entry) )
     {
-        *t = entry.p2m.type;
+        if ( t )
+            *t = entry.p2m.type;

         if ( a )
             *a = p2m_mem_access_radix_get(p2m, gfn);

Cheers,

--
Julien Grall